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

Prevent Future.parallel from cancelling settled computations #124

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

Avaq
Copy link
Member

@Avaq Avaq commented Jul 6, 2017

This was a terribly difficult bug to track down. You may be interested to see this @stevenyap:

  • Future.parallel sends a cancel signal to futures that reject
  • Some _fork methods cannot deal with that. In particular: Future.hook does not expect to be cancelled right after being rejected, and this lead to an unwanted resolution signal being sent.
  • The resolution signal would cause problems in the sequence interpreter. In particular: The interpreter would already be "done" and cleaned up its state when receiving the unexpected resolution signal. This in turn would cause it to try and re-resolve the current action, which finally left us with action.resolve throwing an error.

Fixes #123

@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #124 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #124   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          47     47           
  Lines        1086   1088    +2     
=====================================
+ Hits         1086   1088    +2
Impacted Files Coverage Δ
src/parallel.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3905da6...720b298. Read the comment docs.

@Avaq Avaq merged commit 9fc180d into master Jul 6, 2017
@Avaq Avaq deleted the avaq/fix-parallel-cancellation branch July 6, 2017 13:55
@davidchambers
Copy link
Contributor

Nice pull request description, Aldwin! Making such notes contemporaneously is a good idea. :)

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.

Undefined action in interpretor
3 participants