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

Handle Possibility that Parent Scope is Closed #3912

Merged
merged 2 commits into from Jun 30, 2020

Conversation

adamgfraser
Copy link
Contributor

Resolves #3910.

In FiberContext#fork there is an implicit assumption that unsafeEnsure will always succeed in adding a finalizer to the parent scope. However, that does not appear to always be the case. In particular, after fork is called but before unsafeEnsure obtains a lock, a concurrent process could close the parent scope. Right now this causes a defect which this PR removes.

In addition to this, I think we have a risk of not propagating interruption, because if the parent scope has already been closed we want to immediately interrupt the child fiber or possibly not begin executing it at all.

@adamgfraser adamgfraser requested a review from jdegoes June 29, 2020 17:01
@jdegoes
Copy link
Member

jdegoes commented Jun 29, 2020

fork can only be called on the fiber: it can't be called externally. So if it's called at all, the fiber must be running. It will keep running until after fork is called and the main interpretation loop continues.

Are you sure we're hitting this case?

@adamgfraser
Copy link
Contributor Author

We're definitely hitting that case in the example from the issue. I added a debug statement for that, published a local version of ZIO, depended on that in the reproducer from the original issue, and verified that we were hitting that case.

Correct me if I'm wrong, but if we are using an Executor with an underlying thread pool with multiple threads, couldn't each of them be executing the main interpretation loop at the same time for different fibers? So say first thread is interpreting fiber A and begins executing fork with scope X. At the same time a second thread is executing fiber B, has a reference to scope X and closes it. First thread continues its interpretation of fork and tries to add the finalizer to the scope, which is now closed.

@jdegoes
Copy link
Member

jdegoes commented Jun 30, 2020

Correct me if I'm wrong, but if we are using an Executor with an underlying thread pool with multiple threads, couldn't each of them be executing the main interpretation loop at the same time for different fibers?

No. The main interpretation loop for a given FiberContext (evaluateNow) will only ever be executed by one thread. Within the life of a single fiber, it's executor is "single threaded". The fork operation should only be called from the fiber itself, as it interprets Fork and RaceWith instructions. If that's not happening, if somehow evaluateNow is being executed concurrently, well that would be a serious error which would lead to not only this problem but many others (including random class cast exception errors and stack errors as two threads concurrently pull continuations from the fiber's single stack).

Maybe there is some bug around termination conditions which is causing a fiber already being executed to do some fork, but if that's the case, we should fix that bug rather than work around it here.

@adamgfraser
Copy link
Contributor Author

Right, but two different threads could be executing the interpretation loops for two different fibers. And one fiber could have a reference to the scope of the other fiber since there are all these ways to pass around scopes now. So the scope that one fiber is acting on could be closed part way through.

@adamgfraser adamgfraser merged commit c4ccce5 into zio:master Jun 30, 2020
@adamgfraser adamgfraser deleted the 3910 branch July 27, 2020 18:51
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.

RC21-1 The fiber's scope has ended before the fiber itself has ended
2 participants