Fix race conditions due to MVar usage #5
Conversation
alt-romes
force-pushed
the
wip/fix-mvar
branch
2 times, most recently
from
fb46969 to
aa902fb
Compare
The lock `dap` takes on the MVar could cause a nasty deadlock while
doing something seemingly standard in the debugger threads that are
launched by registerNewDebugSession.
Consider
(readDebuggerOutput, writeDebuggerOutput) <- liftIO P.createPipe
registerNewDebugSession (maybe "debug-session" T.pack __sessionId) DAS{..}
[ debuggerThread ... writeDebuggerOutput
, outputEventsThread readDebuggerOutput
]
An outline of the deadlock is:
1. Initialisation starts, starts writing to a pipe
2. Worker thread tries to output things from the pipe, but can't because that requires taking a lock on the MVar
3. Step 1 blocks because the pipe gets full
4. Step 2 blocks, since it is waiting for step 1 to finish
We fix this by getting rid of the `MVar` internally and adding a
`ReaderT` to the stack transformer with a debugger-server-wide
sessionId backed by an IORef.
`Adaptor` now has an additional type parameter denoting the type of the
request we are responding to. Crucially, this will be `Request` when
responding to a DAP request (e.g. in `send***` functions). On the other
hand, this will be `()` for the `withAdaptor` continuation argument of
`registerNewDebugSession` which unlifts `Adaptor` to `IO` because, when
unlifting, we are not replying to any request.
These changes to the internal implementation of `Adaptor` which allow us
to get rid of the `MVar` are sufficient to fix the deadlock and now
avoid footguns uses of `withAdaptor`.
Fixes haskell-debugger#6
|
@dmjio I've fixed the testsuite now! |
|
@alt-romes @mpickering , the solution seems reasonable to me. Let me ensure that this fix is compatible with the haskell-estgi-debugger, ideally this library will be general enough to support the needs of both. I will say that the MVar you're referring to is used heavily in the estgi-debugger. But if we can express the same logic there as you have here, that would be ideal. Do you have a link to the usage of |
|
Thanks @dmjio. Please take your time. We've made the source public at https://github.com/well-typed/ghc-debugger, even though the debugger is not quite ready for the general public yet. We'll happily let everyone know once we have a first version. We believe the I would also like to point out that I'd be happy to be involved in the maintenance and release process of Thanks again! |
|
@alt-romes, would be happy to collaborate. I can add you as a maintainer, along with @mpickering as well, on Github and Hackage. @csabahruska and I would be happy to partner on this. Exciting you guys are using this for GHC. That's very cool. I'll go ahead and add you guys, but for now, regarding this PR, would it be possible to reinstate the This way we can keep development on the mainline branch and it should unblock things, and allow releases for you guys. |
|
Happy to be on board! I could try to reinstate the Therefore, I suggest we leave this be while you find time to take a deeper look at estgi and figure out if it is really necessary to have the MVar (at which point we can have the In the meantime I may put up other MRs rebased on this one, and then I'll rebase them once this lands. No hurries. |
|
You should now have hackage rights, GitHub maintanership, and an invite to the org @haskell-debugger |
|
@alt-romes sounds good to me, I'll repost when I can take a deeper look. Thank you. |
|
@dmjio I tried to update haskell-estgi-debugger but building it was complicated because of souffle-haskell c++ code that doesn't build properly. I was wondering if you'll have some time to take a look at updating it in the near future. We are planning a release for an alpha version of the GHC debugger and we'd appreciate it if an updated Note: we have one or two more patches on |
|
@alt-romes I'll take a look at it. I'm planning on abstracting out the The souffle build is tough, will try to tackle it with nix. |
This seems fine. Feel free to push commits on top of mine in this branch |
- Renames new 'r' type variable to be 'request' (for library consumers) - Uses 'throwError' instead of 'error' when using the new 'sendEvent' function - Bumps nix to use ghc966 in shell.nix - Light formatting updates
|
@alt-romes I went ahead and pushed to your branch with some light housekeeping-related changes. Now that we have grin-compiler/ghc-whole-program-compiler-project#13 and haskell-debugger/haskell-estgi-debugger#23, the @csabahruska is currently working on a dependently-typed version of Haskell that has a typed-macro system, which he is considering porting some of the logic from the So moving forward I think we can merge these changes and not worry about |
|
I've seen your many patches across the board. Great work. I'm happy to proceed as you described if you think that's a good path forward for Csaba's project sounds interesting too, good luck. I'll be on the lookout for news. Land at will! I'll rebase and push the subsequent patches tomorrow. |
|
Thanks for the kind words @alt-romes. Sounds good will be on the lookout for additional patches tomorrow. If interested here is the typed-macro system Haskell project (based on 2LTT) |
Consider the deadlock described in #6
We fix this by getting rid of the
MVarinternally and adding aReaderTto the stack transformer with a debugger-server-wide sessionId backed by an IORef.Adaptornow has an additional type parameter denoting the type of the request we are responding to. Crucially, this will beRequestwhen responding to a DAP request (e.g. insend***functions). On the other hand, this will be()for thewithAdaptorcontinuation argument ofregisterNewDebugSessionwhich unliftsAdaptortoIObecause, when unlifting, we are not replying to any request.These changes to the internal implementation of
Adaptorwhich allow us to get rid of theMVarare sufficient to fix the deadlock and now avoid footguns uses ofwithAdaptor.Fixes #6