search

brunoczim / fslock

File locking for Rust.
MIT License
40 stars 11 forks source link

Allow users to be able to retain lockfiles #8

Closed ctrlcctrlv closed 2 years ago

ctrlcctrlv commented 2 years ago

Without breaking any old API.

Needed by the MFEK project. We will maintain a fork if PR rejected.

ctrlcctrlv commented 2 years ago

@brunoczim I do think that my Into<std::fs::File> API is needed. Without it, users have to hold two handles which breaks some OS guarantees. See https://github.com/MFEK/metadata/commit/82f341cdab56ca30e937b5384435e73172e62478 for how I'm using this in production right now.

ctrlcctrlv commented 2 years ago

Also regarding the FileLock::raw() API that's used here https://github.com/MFEK/metadata/commit/6396812bdebe419c53b962c6b83efb6deade34a7

As I stated in https://github.com/brunoczim/fslock/pull/8#discussion_r788792940 I wrote this expecting upstream rejection (which you're still free to do) because maybe it's too close to my project's needs and not general needs.

brunoczim commented 2 years ago

@brunoczim I do think that my Into<std::fs::File> API is needed. Without it, users have to hold two handles which breaks some OS guarantees. See MFEK/metadata@82f341c for how I'm using this in production right now.

Why do you say a user would want two handles? You shouldn't be reading or writing to a lockfile.

ctrlcctrlv commented 2 years ago

@brunoczim It's a locked file in my use not a lockfile.

ctrlcctrlv commented 2 years ago

Sorry, we seem to have overwrote one another. I did a rebase to remove the change I made to Cargo.toml, but you removed the whole field. I'll rv back.

brunoczim commented 2 years ago

I do think that allowing PID-retaining makes sense, but I am not sure I'd add it to fslock by my own initiative. However, if you really want to merge this feature, I can help you adjust a few things I would require.

ctrlcctrlv commented 2 years ago

@brunoczim This isn't only for PID-retaining. The problem that spurred this change was a very MFEK problem: multiple instances of MFEKglif were calling MFEKmetadata to overwrite layercontents.plist of the same font. MFEKmetadata, as a side effect of any edit it makes to a UFO font, rewrites metainfo.plist to leave its "calling card" (a/k/a leaves itself as the program to blame for a bad change). What this meant was that in some cases, because metainfo.plist wasn't being locked and unlocked, it was getting corrupted (two writes on top of one another at different offsets, many other similar issues).

So I used your crate to lock metainfo.plist, get its file handle, write the data I wanted, then mem::forget the std::fs File leaving LockFile responsible for the close.

ctrlcctrlv commented 2 years ago

That's why I said, my changes make the crate both for (write-)locked files and lockfiles, which are different beasts.

brunoczim commented 2 years ago

Yeah... I think this goes beyond fslock's scope. fslock was intended to use files simply as locks and not have meaningful content inside them. In fact, the only expected content is a PID (related to locking). So, I don't want to add write-lock features to this library. Given that, I think I should reject this indeed. If you'd like to make another contribution - or even a new version of this PR - feel free.

ctrlcctrlv commented 2 years ago

@brunoczim I'd be willing to change anything about this PR except the ultimate goal of being able to write meaningful non-PID contents to the files.

But given you don't want that, I'll maintain a fork: https://github.com/MFEK/fslock.rlib

However, if I do something in the fork I think has relevance upstream, and I haven't diverged too much, I'll PR, sure.