brunoczim / fslock

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

WIP: Feature to enable uniform behavior for unix, windows #3

Closed nmathewson closed 3 years ago

nmathewson commented 3 years ago

This patch tries to gives Unix the same behavior as is documented for Windows when opening multiple handles to the same file: if any other Lockfile currently is locking the same file, then other Lockfiles will block when trying to lock it.

It works by using an internal HashMap to determine which files are locked and which are not. (We identify files by a combination of st_dev and st_ino). This logic is behind a new feature, multilock, which depends on std and once_cell.

I'm submitting this patch for feedback, to ask if the general approach looks like something that could be accepted. Please don't merge it as-is: it needs tests and documentation.

nmathewson commented 3 years ago

Friendly ping: does this look like something worth pursuing? If so, I can continue improving it to a state you want.

brunoczim commented 3 years ago

Sorry for the delay btw, my GH notifications are somewhat messy

nmathewson commented 3 years ago

Thanks for the review! I agree with all your suggestions; I'll prepare a fresh branch with these changes, along with a set of tests and documentation updates.

nmathewson commented 3 years ago

There's one issue I'm running into: on revision, I don't think that it is correct to use a Cargo feature to turn this behavior on and off.

The issue is that some existing software may be depending on the current behavior, but if it is linked with another crate that turns on the multilock feature, it will get the new behavior without wanting it.

I think adding this behavior in a 100% backward-compatible way will require a new API, so I'll try coding it that way. Please let me know if you'd prefer something different, or you have any preference for the naming.

brunoczim commented 3 years ago

It is ok for me to have a new API. I mean, we are at a zero major version, which assumes a higher risk of breakage. Of course it depends on the actual API, but I am sure we can work on it.

nmathewson commented 3 years ago

I've tried again; I'm not sure about about the new API, so I'd like your feedback.

(Is open_excl a good name?)

(Do you think that it's still worthwhile to have a separate multilock feature, or should open_excl be available whenever any(not(unix), feature="std") ? )

nmathewson commented 3 years ago

I've made the requested changes to the internal names.

The main reason that I was unsure about open_excl is the possibility of confusion between this notion of "exclusvity" with the notion of "exclusivity" from the flock(2) system call's LOCK_EX flag. But if you think it's fine, I'm happy to leave the name as-is.

(If you merge this, could it please get a new release? I'd like to start using it in production.)

brunoczim commented 3 years ago

The main reason that I was unsure about open_excl is the possibility of confusion between this notion of "exclusvity" with the notion of "exclusivity" from the flock(2) system call's LOCK_EX flag

Hmmmm I see, I might just clarify it on the docs before releasing, but I think it is OK anyway. Yes, I will release it soon!

nmathewson commented 3 years ago

Thanks for all your help here, and for maintaining this crate!