fix logic error and race condition #8
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were both a race condition and logic error with the implementation, which could lead to spurious signals, crashes, and other undefined behaviour. Race Condition -------------- Thread 1: ... 1. `suspend`: acquires lock for actor `A` and pool 2. changes the state to `Suspended` 3. `suspend`: releases the locks 4. pauses Thread 2: .... 1. `resume`: acquires lock for actor `A` and pool 2. actor state is `Suspended`, so the actor is queued 3. `resume`: releases the locks 4. ... 5. actor `A` is popped from the work queue and resumes execution Both thread 1 and 2 now have unguarded concurrent ownership of actor `A`, which constitutes a race condition. Logic Error ----------- Note: this is only *one* example of what could happen. Thread 1: ... 1. `suspend`: acquires lock for actor `A` and pool 2. changes the state to `Suspended` 3. releases the lock for actor `A` and and pool 4. pauses Thread 2: ... 1. `resume`: acquires lock for actor `A` and pool 2. actor state is `Suspend`, so the actor is queued 3. ... 4. actor `A` resumes execution 5. `jield` is called, and the state of actor `A` changes to `Jielding` 6. ... 7. `workerThread`: fetches the state of actor `A` (`Jielding`) 8. pauses Thread 1: 1. `workerThread`: fetches the state of actor `A` (`Jielding`) 2. queues actor `A` for execution and changes the state to `Queued` 3. pauses Thread 2: 1. the local `state` variable still contains `Jielding`, so the actor is queued again The same actor is now in the work queue two times! The Solution ------------ In both cases, the problem is that another thread can assume ownership of an actor while the worker thread responsible for the actor hasn't finished suspending it, due to the actor state being eagerly set to `Suspended` by `suspend`. `suspend` now sets the state to the new `Suspending` state first. When handling the `Suspending` state, if new signals were received in the meantime (`sendSig` cannot resume a *suspending* actor), the actor is queued again right away. Otherwise, it's suspended.
|
Thanks for this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There were both a race condition and logic error with the implementation, which could lead to spurious signals, crashes, and other undefined behaviour.
Race Condition
Thread 1:
...
suspend: acquires lock for actorAand poolSuspendedsuspend: releases the locksThread 2:
....
resume: acquires lock for actorAand poolSuspended, so the actor is queuedresume: releases the locksAis popped from the work queue and resumes executionBoth thread 1 and 2 now have unguarded concurrent ownership of actor
A, which constitutes a race condition.Logic Error
Note: this is only one example of what could happen.
Thread 1:
...
suspend: acquires lock for actorAand poolSuspendedAand and poolThread 2:
...
resume: acquires lock for actorAand poolSuspend, so the actor is queuedAresumes executionjieldis called, and the state of actorAchanges toJieldingworkerThread: fetches the state of actorA(Jielding)Thread 1:
workerThread: fetches the state of actorA(Jielding)Afor execution and changes the state toQueuedThread 2:
statevariable still containsJielding, so the actor is queued againThe same actor is now in the work queue two times!
The Solution
In both cases, the problem is that another thread can assume ownership of an actor while the worker thread responsible for the actor hasn't finished suspending it, due to the actor state being eagerly set to
Suspendedbysuspend.suspendnow sets the state to the newSuspendingstate first. When handling theSuspendingstate, if new signals were received in the meantime (sendSigcannot resume a suspending actor), the actor is queued again right away. Otherwise, it's suspended.