Various MVar fixes #70
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
dcoutts
force-pushed
the
dcoutts/mvar-fixes
branch
from
0b27604 to
02da1a3
Compare
dcoutts
force-pushed
the
dcoutts/mvar-fixes
branch
from
02da1a3 to
6334fa3
Compare
dcoutts
force-pushed
the
dcoutts/mvar-fixes
branch
from
4f978b5 to
c7ffcc0
Compare
In the default implementation of MVar (used in the SimM, not IO), there were two cases where the mvar state was not being correctly updated. In both takeMVarDefault and tryTakeMVarDefault, in the case of a full mvar with an empty wait queue, the mvar state was not being updated. This is likely to be the cause of deadlocks observed in some simulations using MVars.
The existing code assumed for some reason that it is impossible to have any threads waiting on an empty mvar when someone is testing it to see if it is empty. This is false, it's entirely possible, and doesn't affect the question of whether the mvar is empty or not.
Scheduling means we cannot know the state of the MVar in the cleanup handlers, so we cannot assume the mvar is empty/full. So don't fail in such cases.
We don't require this for TVar or other classes. We only currently use injective type families for the monad (like STM) not the variables. It makes it harder (in the current style) to add newtype instances. Fix tests (by removing unnecessary type annotations). These type annotations could have done the intended thing, but only with scoped type variables (which was not used). Easier to remove them.
Into a more logical grouping for review and modification.
Rename 'wakevar' to either 'takevar' or 'putvar', depending on usage. Rename 'blockedq' to either 'takeq' or 'putq', depending on usage. This helps to clarify the code because both takeMVar and putMVar have to deal with both queues, so calling them the same hinders clarity. Similarly (to a lesser extent) the wake variables. This will be especially helpful when we introduce a third queue for threads blocked on readMVar: the 'readq'.
Use a second readq of threads blocked on readMVar, which gets woken up by putMVar or tryPutMVar. All "readers" get woken up and the first "taker" if any.
It provides the wrong semantics, so it's not a helpful default
Hopefully slightly improve clarity.
It's actually not a suitable general purpose implementation, but it is ok for the simulation.
Suggestion from code review.
jorisdral
force-pushed
the
dcoutts/mvar-fixes
branch
from
343b696 to
71f6cb2
Compare
17 tasks
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.
takeMVarthat was probably causing deadlocksisEmptyMVartakeMVar/putMVarin async exception caseMVartype family to be injective, which makes it consistent withTVarand others.readMVarwhich had the wrong semantics.tryReadMVarto the class