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 a bug in Fiber.finalize #3009

Merged
1 commit merged into from Jan 8, 2020
Merged

Fix a bug in Fiber.finalize #3009

1 commit merged into from Jan 8, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 7, 2020

This fixes #2958.

The problem was the finally was called too late in case of error. I also removed wait_errors from the API as it's unused and it seems like it could cause the same issue.

@aalekseyev
Copy link
Collaborator

Hmm, I find both versions of this function suspicious.

Before this PR, finally would run only after all errors are handled, which, as you say, is a bit too late.

After this PR, finally runs just before the first error is handled. If the concurrent fibers continue executing after that error, we have finally running during execution, which I think contradicts the docs.

Maybe a good behavior is to have finalize collect all errors without handling them until all computations are done and then run finally and emit all errors to the surrounding handler?

@ghost
Copy link
Author

ghost commented Jan 7, 2020

Indeed, I hadn't considered we would still have code from f running after after finally has been called. Buffering errors seems like the right thing to do indeed. Doing that.

Fix #2958

Signed-off-by: Jeremie Dimino <[email protected]>
@ghost
Copy link
Author

ghost commented Jan 7, 2020

I reworked the code. The result is probably not as efficient as it could be, but at least the implementation of finalize is straightforward.

I also changed collect_errors so that the errors are reraised in the same order as they were originally raised.

Copy link
Collaborator

@aalekseyev aalekseyev left a comment

Choose a reason for hiding this comment

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

Looks good

@ghost ghost merged commit 78495a0 into ocaml:master Jan 8, 2020
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Jan 8, 2020
…lugin, dune-private-libs and dune-glob (2.1.2)

CHANGES:

- Fix a bug in the `Fiber.finalize` function of the concurrency monad of Dune,
  causing a race condition at the user level (ocaml/dune#3009, fix ocaml/dune#2958, @diml)
This pull request was closed.
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.

dune 2.0.0: promotion stops after first rule failure
2 participants