Shan/concurrent-cache-rewrite#171
Conversation
| import UnliftIO.MVar (MVar) | ||
| import UnliftIO.MVar qualified as MVar | ||
|
|
||
| data CurrentFileCache = CurrentFileCache |
There was a problem hiding this comment.
Lets rename this to FileCache instead (the type itself could apply to any file)
josephsumabat
left a comment
There was a problem hiding this comment.
You use modifyMVar_ pretty extensively in this PR - this is generally not thread safe. Consider the description:
An exception-safe wrapper for modifying the contents of an [MVar](https://hackage.haskell.org/package/base-4.21.0.0/docs/Control-Concurrent-MVar.html#t:MVar). Like [withMVar](https://hackage.haskell.org/package/base-4.21.0.0/docs/Control-Concurrent-MVar.html#v:withMVar), [modifyMVar](https://hackage.haskell.org/package/base-4.21.0.0/docs/Control-Concurrent-MVar.html#v:modifyMVar) will replace the original contents of the [MVar](https://hackage.haskell.org/package/base-4.21.0.0/docs/Control-Concurrent-MVar.html#t:MVar) if an exception is raised during the operation. This function is only atomic if there are no other producers for this [MVar](https://hackage.haskell.org/package/base-4.21.0.0/docs/Control-Concurrent-MVar.html#t:MVar). In other words, it cannot guarantee that, by the time [modifyMVar_](https://hackage.haskell.org/package/base-4.21.0.0/docs/Control-Concurrent-MVar.html#v:modifyMVar_) gets the chance to write to the MVar, the value of the MVar has not been altered by a write operation from another thread.
So it's possible that another thread is able to overwrite the MVar
I want you to think further about each function and the "ownership" of the cache. For example if you request a definition we don't need to fully claim ownership and can use tryReadMVar so that we also don't block invalidating the cache, but if we make an edit we do want to claim ownership and ensure nothing else overwrites/we don't overwrite another edit.
This PR currently has issues if you go-to-def on a new file, edit the original, things stop working
| case maybeCache of | ||
| Nothing -> do | ||
| switchToFile path | ||
| getFileState path |
There was a problem hiding this comment.
Avoid recursion here - just return the value directly
No description provided.