-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simulate file locks in blockio-sim
#415
base: main
Are you sure you want to change the base?
Conversation
The simulation piggybacks on existing `HasFS` functionality for opening files in different `OpenMode`s. The simulation does not simulate inter-process locking.
dba71e5
to
8eb3c61
Compare
With the nice addition that we now assert that there are no open file handles at the end of each iteration of the tests!
8eb3c61
to
3e19457
Compare
3e19457
to
63d7868
Compare
It's better but not perfect. It passes the test, but would fail if there were genuine concurrency. Doing it properly would need an implementation in fs-api and fs-sim. Fortunately we do not need a concurrency safe implementation (for now).
Follow a more normal convention, and allow running individual tests more easily in GHCi via quickCheck $ theProperty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcoutts looks great
-- | ||
-- We implement this using the content of the lock file. The content is a | ||
-- counter, positive for readers and negaive (specifically -1) for writers. | ||
-- There can be any number of readers, but only one writer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- There can be any number of readers, but only one writer. | |
-- There can be any number of readers, but only one writer. Writers can not coexist with readers. |
-- | ||
-- Warning: This implementation is not robust under concurrent use (because | ||
-- operations on files are not atomic) but should be ok for casual use. A | ||
-- proper implementation would need to be part of the underlying 'HasFs' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-- proper implementation would need to be part of the underlying 'HasFs' | |
-- proper implementation would need to be part of the underlying 'HasFS' |
-- Warning: This implementation is not robust under concurrent use (because | ||
-- operations on files are not atomic) but should be ok for casual use. A | ||
-- proper implementation would need to be part of the underlying 'HasFs' | ||
-- implementations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
readCount :: Handle h -> m Int | ||
readCount h = do | ||
content <- API.hGetSome hfs h 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use hGetAllAt
or something similar, because hGetSome
can do partial reads
writeCount :: Handle h -> Int -> m () | ||
writeCount h n = do | ||
API.hSeek hfs h AbsoluteSeek 0 | ||
_ <- API.hPutSome hfs h (BS.pack (show n)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should hPutAllStrict
or something similar, because hPutSome
can do partial writes
-- | Lock files are reader\/writer locks. | ||
-- | ||
-- We implement this using the content of the lock file. The content is a | ||
-- counter, positive for readers and negaive (specifically -1) for writers. | ||
-- There can be any number of readers, but only one writer. | ||
-- | ||
-- Warning: This implementation is not robust under concurrent use (because | ||
-- operations on files are not atomic) but should be ok for casual use. A | ||
-- proper implementation would need to be part of the underlying 'HasFs' | ||
-- implementations. | ||
-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also maybe warn that the simulation only defines how tryLockFile
s on the same path interact. A regular hOpen
on the same path will still work
What's neat is that we can now find out in our
StateMachine
tests whether our implementation is leaking file descriptors!TODO: the test I added shows that my implementation of file locks is not right just yet... Two
ExclusiveLock
s work fine, butSharedLock
s don't work. I was assuming that mocked file handles don't allow concurrent read and write locks, but it seems they do, so we can't just piggyback on theOpenMode
s