Skip to content
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

Support locking of directory entries #139

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nathanmemmott
Copy link
Contributor

@nathanmemmott nathanmemmott commented Jul 6, 2023

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

Fixes #137


Preview | Diff

@a-sully a-sully self-requested a review July 11, 2023 15:24
Copy link
Collaborator

@a-sully a-sully left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nit

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@a-sully a-sully self-requested a review July 11, 2023 15:33
index.bs Outdated Show resolved Hide resolved
@a-sully a-sully self-requested a review July 11, 2023 18:12
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@a-sully a-sully self-requested a review July 13, 2023 02:17
Copy link
Collaborator

@a-sully a-sully left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@a-sully a-sully requested a review from annevk July 13, 2023 17:56
@a-sully a-sully mentioned this pull request Jul 17, 2023
3 tasks
@a-sully
Copy link
Collaborator

a-sully commented Aug 15, 2023

cc @jesup @szewai @annevk

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

@nathanmemmott
Copy link
Contributor Author

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

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me, but I'm not entirely comfortable signing off for WebKit. I'd like at least @szewai to weigh in as well.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved

1. Let |lock| be the |file|'s associated [=file entry/lock=].
1. Let |count| be the |file|'s [=file entry/shared lock count=].
Note: These steps have to be run on the [=file system queue=].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes more sense to note at the bottom of "take a lock" as that's the one that will be invoked directly from elsewhere, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is noted at the bottom of "take a lock". Should it only be noted there?

@szewai
Copy link

szewai commented Nov 8, 2023

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
Copy link
Contributor Author

Mistakenly closed while cleaning up branches.

@nathanmemmott nathanmemmott reopened this Dec 9, 2023
@nathanmemmott
Copy link
Contributor Author

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
Copy link
Contributor Author

Latest push adds a missing check for ancestor locks.

Nathan Memmott added 3 commits December 19, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support locking of directory entries
4 participants