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

fix: run blocking function on a different task #2570

Merged
merged 1 commit into from Jun 26, 2019

Conversation

4 participants
@jcao219
Copy link
Contributor

commented Jun 23, 2019

This avoids freezing the current task if the fn blocks indefinitely

Context:

I'm adding ops to Deno for filesystem watching.

Unfortunately, the cross-platform rust crate for this (rust notify) currently exposes a non-futures API for receiving events. I noticed in test cases that while it is blocking, timers no longer fire.

Example (https://github.com/jcao219/deno/blob/pr-add-watch/js/watch_fs_test.ts):

test(async function watcherClosedWhileWatching(): Promise<void> {
  const watcher = Deno.watch([]);
  const promise = watcher.next();
  setTimeout((): void => watcher.close(), 10); // Never fires...
  const { value, done } = await promise;
  assert(done);
  assertStrictEq(value.eventType, "watcherClosed");
  assertStrictEq(value.source, null);
  assertStrictEq(value.destination, null);
});

This change fixes the issue.

@CLAassistant

This comment has been minimized.

Copy link

commented Jun 23, 2019

CLA assistant check
All committers have signed the CLA.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2019

@jcao219 🙏 kudos!

I had exactly same problem working on #2535, where using timeouts with workers froze everything.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@jcao219 could you sign CLA?

fix: run blocking function on a different task
This avoids freezing the current task if the fn blocks indefinitely

@jcao219 jcao219 force-pushed the jcao219:blocking branch from 3136c65 to e57d904 Jun 26, 2019

@jcao219

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

@bartlomieju Done. :)

@ry

ry approved these changes Jun 26, 2019

Copy link
Collaborator

left a comment

LGTM - thanks @jcao219 !

@ry ry merged commit fb6d57a into denoland:master Jun 26, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.