Clarify ordering of KVStore remove(lazy=true) before write
#4113
Conversation
This is used by the new logic in `lightning-persister` so its best to clarify it. Its already implemented in our filesystem-backed store.
|
👋 Thanks for assigning @joostjager as a reviewer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4113 +/- ##
==========================================
- Coverage 88.72% 88.69% -0.04%
==========================================
Files 177 177
Lines 133286 133286
Branches 133286 133286
==========================================
- Hits 118253 118212 -41
- Misses 12326 12361 +35
- Partials 2707 2713 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// this method must write the new state. In other words, if a call to this method is made with | ||
| /// the `lazy` flag and a call to [`Self::write`] is made after this method returns (but before | ||
| /// the returned future completes), the contents from [`Self::write`] must ultimately be stored, | ||
| /// overwriting the removal. |
There was a problem hiding this comment.
Its already implemented in our filesystem-backed store.
Looking at the code again, is this actually the case? If remove is lazy, according to docs, there is no guarantee that the file is immediately deleted. So what happens if a write comes before the file is actually deleted?
Or is this correct because fs::remove_file does have an immediate effect that ensures that a subsequent write will always create a new file?
There was a problem hiding this comment.
Looking at the code again, is this actually the case? If remove is lazy, according to docs,
there is no guarantee that the file is immediately deleted. So what happens if a write comes before the file is actually deleted?
AFAIU we can't lean on that, especially since as with everything filesystem-related, the behavior might vary from OS to OS.
There was a problem hiding this comment.
Or is this correct because fs::remove_file does have an immediate effect that ensures that a subsequent write will always create a new file?
Yes, removing the file itself has immediate effect, it just may not be written to disk immediately, so on a crash the file may reappear.
There was a problem hiding this comment.
The "no guarantee that the file is immediately deleted" reference in the remove_file docs is made somewhat clearer by the parenthetical - the storage space on disk my still be held if there are open file descriptors, but that doesn't mean the file still exists at the provided path. The Unix unlink terminology makes this somewhat clearer.
There was a problem hiding this comment.
Yes, e.g., depending on platform, other open file descriptors may prevent immediate removal. "E.g.", not sure if we can work with that.
Yes, removing the file itself has immediate effect, it just may not be written to disk immediately, so on a crash the file may reappear.
But it is guaranteed that if we, before crashing, create a new file, that will be the one in existence after coming back up?
There was a problem hiding this comment.
But it is guaranteed that if we, before crashing, create a new file, that will be the one in existence after coming back up?
Yes, because when we write we fsync the written file. Basically a reasonable mental model for the FS here is that all writes/removes/etc are just queueing an operation in memory and fsync says "take everything queued and just write the latest state, blocking until its on disk".
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
There was a problem hiding this comment.
Re-reading the additional assumptions, I'm really questioning the benefit of the lazy flag at all. Its utility was always not entirely clear mod some cloud environments where you actually could save an explicit call in some scenarios. However, I doubt we could make use of this given the additional constraints.
TLDR: can we just drop the lazy flag?
Now opened #4116 as an alternative to this PR. |
|
We did #4116 instead. |
4 participants
This is used by the new logic in
lightning-persisterso its best to clarify it. Its already implemented in our filesystem-backed store.