Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REPL unblock event loop AND fix REPL setTimeout fire problems #1233

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

kevinkassimo
Copy link
Contributor

@kevinkassimo kevinkassimo commented Nov 28, 2018

Closes #1161 .

  • Unblocks event loop by making REPL async
  • Fix op_repl_readline always blocking problem that breaks setTimeout fire
  • Add setTimeout test

/cc @hayd @cedric05

Also, though not fixed in this PR, there is one more issue I found that might need to be address later:

deno/src/resources.rs

Lines 335 to 345 in 89096c9

pub fn readline(rid: ResourceId, prompt: &str) -> DenoResult<String> {
let mut table = RESOURCE_TABLE.lock().unwrap();
let maybe_repr = table.get_mut(&rid);
match maybe_repr {
Some(Repr::Repl(ref mut r)) => {
let line = r.readline(&prompt)?;
Ok(line)
}
_ => Err(bad_resource()),
}
}

Since let line = r.readline(&prompt)?; is blocking, this might cause RESOURCE_TABLE.lock() not released in REPL and blocks other resources reads and writes

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks good - just one comment.

tools/repl_test.py Outdated Show resolved Hide resolved
@ry
Copy link
Member

ry commented Nov 28, 2018

Also please split the commits into e5f204f and yours.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ry ry merged commit 09aa9b9 into denoland:master Nov 28, 2018
# Print after 0.1 second
p.stdin.write(
"setTimeout(() => console.log('HI'), 100)\n".encode("utf-8"))
sleep(0.2) # Wait 0.2 second before proceed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be done with the existing input method, without "special treatment" 😄, and avoid the repetition. I will make a separate PR.

ry added a commit to ry/deno that referenced this pull request Nov 30, 2018
- Allow async functions in REPL (denoland#1233)
- Handle Location header relative URI (denoland#1240)
- Add deno.readAll() (denoland#1234)
- Add Process.output (denoland#1235)
- Upgrade to TypeScript 3.2.1
- Upgrade crates: tokio 0.1.13, hyper 0.12.16, ring 0.13.5
@ry ry mentioned this pull request Nov 30, 2018
ry added a commit that referenced this pull request Dec 1, 2018
- Allow async functions in REPL (#1233)
- Handle Location header relative URI (#1240)
- Add deno.readAll() (#1234)
- Add Process.output (#1235)
- Upgrade to TypeScript 3.2.1
- Upgrade crates: tokio 0.1.13, hyper 0.12.16, ring 0.13.5
@kevinkassimo kevinkassimo deleted the repl/unblock branch December 27, 2019 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

setTimeout does not work in REPL
3 participants