Review masking and add traces when things don't cancel timely #2768
Conversation
|
Who reviews changes made by the code owner? I need a volunteer once again, thanks @wz1000 @michaelpj @eddiemundo |
michaelpj
left a comment
michaelpj
left a comment
There was a problem hiding this comment.
This is the first time I've looked at the AIO stuff. It makes me a little nervous: this stuff is fiendishly difficult, can we definitely not get away with an existing library?
| -- Masked to ensure that the previous values are flushed together with the map update | ||
| -- update the map | ||
| mask_ $ join $ atomicallyNamed "modifyFileExists" $ do | ||
| join $ mask_ $ atomicallyNamed "modifyFileExists" $ do |
There was a problem hiding this comment.
This change doesn't make sense to me. I think this just makes the mask apply to the atomic execution of the STM block, which is already atomic. Previously it ensured that we would perform the effects of the returned action as well without being interrupted. But now I think it doesn't do that, and indeed maybe does nothing?
There was a problem hiding this comment.
It still prevents the STM transaction from being interrupted by an edit, which is important to accurately track file dirtiness.
There was a problem hiding this comment.
But we can end up not calling recordDirtyKeys, since that happens in the IO action afterwards. Is that important? Maybe the comment could say.
There was a problem hiding this comment.
The IO is just logging, recordDirtyKeys mutates the collection inside the STM transaction. That said I appreciate how this is confusing and perhaps the mask_ was better at the top level for clarity
| -> [Key] -- ^ Previous direct dependencies of Id | ||
| -> HashSet Key -- ^ Current direct dependencies of Id | ||
| -> IO () | ||
| -- mask to ensure that all the reverse dependencies are updated |
There was a problem hiding this comment.
This one I wonder if we could avoid by pushing the scope of the STM transaction out a bit?
There was a problem hiding this comment.
STM transactions can still be interrupted by exceptions, so the question is how badly do we want to record the reverse deps. The answer is very badly - failure to do so will lead to unsoundness, specially if this is the first build.
EDIT: oh, correcting myself, since we don't mark the build as done until after the reverse keys have updated, I don't think that the mask is needed at all.
| unless (null asyncs) $ do | ||
| let loop = forever $ do | ||
| sleep 10 | ||
| traceM "cleanupAsync: waiting for asyncs to finish" |
There was a problem hiding this comment.
Do we really want to use Debug.Trace here?
There was a problem hiding this comment.
If you are seeing this trace, then most likely hls-graph is broken.
Plus, I didn't want to thread a logger around....
There was a problem hiding this comment.
You're in IO, you could just print to stderr. It's a small difference, but I do think not having Debug.Trace in production code is good.
There was a problem hiding this comment.
That's a fair point
|
|
I guess I mean the functions operating on |
|
|
…l#2768) * Review masking and add traces when things don't cancel timely * fixup * use sleep consistently * redundant imports * hlints * fix 9.2 build * Fix 9.2 build for real * remove unnecessary polymorphism * Avoid spawning loop async unnecessrily * flush asyncs ref * Add comments and apply @michaelpj suggestions
No description provided.