search

whatwg / fs

File System Standard
https://fs.spec.whatwg.org/
Other
235 stars 19 forks source link

Support locking of directory entries #139

Open nathanmemmott opened 1 year ago

nathanmemmott commented 1 year ago

Moves the locking algorithm from file entry to file system entry.

Fixes #137

Preview | Diff

a-sully commented 1 year ago

cc @jesup @szewai @annevk

Please see my above comment. Let me know if you have comments of questions

nathanmemmott commented 1 year ago

@jesup @szewai @annevk Have you had a chance to take a look at this?

szewai commented 1 year ago

LGTM with some nits. Thanks for persevering on this change!

I'll stamp my approval, but given that this includes some new behaviors (mainly, that taking a lock on a directory can only succeed if no children are locked) I'd like to get a +1 from someone from another browser vendor. @annevk care to take a look?

Let me know if you have more questions about the rationale for this behavior or its implications. It's discussed briefly here and here. The perhaps unexpected (if you're familiar with Posix, at least) implications are that moving or deleting a directory that has an open FileSystemSyncAccessHandle will fail

Both of these operations "just work" on Posix, but not on other platforms (for example, on Windows you can't delete a directory while there's an open file handle). With the web being a cross-platform platform, I don't think we should be exposing implementation details of Posix

(we should probably add a note in the spec about this...)

(Sorry if I have missed any information) I don't quite understand the rationale of declaring lock on a directory entry. In what cases will we need to take / release the lock? If we just want to ensure no children is locked when directory is removed or moved, we could just perform the check directly before the operation.

I could understand a directory entry might have a "isLocked" state to indicate whether any of its children holds lock, but not sure about a directory itself has a lock.

nathanmemmott commented 11 months ago

Mistakenly closed while cleaning up branches.

nathanmemmott commented 11 months ago

LGTM with some nits. Thanks for persevering on this change! I'll stamp my approval, but given that this includes some new behaviors (mainly, that taking a lock on a directory can only succeed if no children are locked) I'd like to get a +1 from someone from another browser vendor. @annevk care to take a look? Let me know if you have more questions about the rationale for this behavior or its implications. It's discussed briefly here and here. The perhaps unexpected (if you're familiar with Posix, at least) implications are that moving or deleting a directory that has an open FileSystemSyncAccessHandle will fail Both of these operations "just work" on Posix, but not on other platforms (for example, on Windows you can't delete a directory while there's an open file handle). With the web being a cross-platform platform, I don't think we should be exposing implementation details of Posix (we should probably add a note in the spec about this...)

(Sorry if I have missed any information) I don't quite understand the rationale of declaring lock on a directory entry. In what cases will we need to take / release the lock? If we just want to ensure no children is locked when directory is removed or moved, we could just perform the check directly before the operation.

I could understand a directory entry might have a "isLocked" state to indicate whether any of its children holds lock, but not sure about a directory itself has a lock.

Yes this is just for move and remove for right now. Move and remove will be available for both files and directories. We'll need to take some sort of exclusive lock during these operations whether that's a file system entry/lock or isLocked. The file system entry/lock is reused to simplify locking logic. For example, isLocked would have to be added to file system entry/take a lock steps as an alternative way to exclusively lock.

nathanmemmott commented 11 months ago

Latest push adds a missing check for ancestor locks.