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

Operations on deleted FileHandles/DirectoryHandles #39

Closed
jesup opened this issue Jul 25, 2022 · 9 comments
Closed

Operations on deleted FileHandles/DirectoryHandles #39

jesup opened this issue Jul 25, 2022 · 9 comments

Comments

@jesup
Copy link
Contributor

jesup commented Jul 25, 2022

While the WPT tests appear to test a few things with deleted File/Directory handles (like that values() on a deleted directory returns NotFoundError), there's no spec text that I see to specify what happens to an existing File/DirectoryHandle when it's removeEntry()'d from it's parent. Ditto SyncAccessHandles.
Also: what happens if an async method is still in progress?

@jesup
Copy link
Contributor Author

jesup commented Jul 25, 2022

Also don't forget we could have handles in both MainThread and in workers to the same object, and delete it from either

@a-sully
Copy link
Collaborator

a-sully commented Jul 28, 2022

SyncAccessHandles grab an exclusive lock on a file, so removeEntry() currently isn't allowed, since it grabs a shared lock on the file (the same type of lock acquired by createWritable()). This is not currently specified, since the method was around long before we added locking...

removeEntry() probably SHOULD require an exclusive lock (there's a tracking bug in Chrome) but there's currently a WPT asserting that removal with an open FileSystemWritableFileStream succeeds. I think we should change this.

The proposed move() and remove() methods will require grabbing an exclusive lock on the file, so these operations would not be allowed with either an open SyncAccessHandle or FileSystemWritableFileStream

As for the other async operations, isSameEntry() doesn't look at the contents on disk anyways. getFileHandle() and getDirectoryHandle() have a note to "Better specify what possible exceptions this could throw.", and that could reasonably include what to do if the underlying directory has been deleted

@jesup
Copy link
Contributor Author

jesup commented Aug 2, 2022

removeEntry() does not (in the spec currently) take a shared lock on the object being removed. Whether it takes a lock in Chrome at the moment isn't the question.

We need to decide what sort of API we want here (linux-style, access is still possible even if removed, or windows-style, can't remove if open). What we appear to have now is neither - errors are returned after removal.

Most of this issue was around "what happens if we have a FileSystemFileHandle or FileSystemDirectoryHandle, and some other operation removeEntry()'s them." A FileSystemFileHandle/FileSystemDirectoryHandle don't hold any sort of lock per spec, only FileSystemSyncAccessHandles and FileSystemWritableFileStreams

@jesup
Copy link
Contributor Author

jesup commented Aug 4, 2022

I'm going to assume we should take an exclusive lock, and if that fails return InvalidModification

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 9, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

TODO: link blink-dev PSA

Fixed: 1254078
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
@jesup
Copy link
Contributor Author

jesup commented Sep 15, 2022

Re TPAC discussion about deleting/moving files with handles:

What are FileHandles/DirectoryHandles?

If you argue they are abstract paths, then deleting the underlying file/directory would cause an error if we try to use the handle, and if we were to move the file/directory, we'd also get an error (unless we moved another file into the spot first). (I believe Chrome has implemented this.)

If you argue they are references to objects resolved at creation time, then deleting the underlying file (from the owning directory) will cause an error if we try to use the file. On the other hand, if we move the file/directory, one would expect that the File/Directory handle would still be usable (and if you call resolve() on it you'd get the new path). (Right now Firefox does this; the internal DB id's are stable across rename.)

So, which is it? I should note that the fact that createFileHandle() takes a create: true option (and fails without that if it doesn't exist) implies that it's not purely an abstract path.

Note another option for remove(): if this was unix-posix-like semantics, removing a file with an active reference would not impact the ability to use existing handles to the removed item.

Once we have a final decision on this, we need to adjust all the spec language to specify what happens in these cases.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 17, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

TODO: link blink-dev PSA

Fixed: 1254078
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 18, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

TODO: link blink-dev PSA

Fixed: 1254078
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 19, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

Also updates the FSUnderlyingSink to reset its mojo connection when the
stream reaches an errored state. WritableStreams are no longer usable
after an error, so resetting the remote releases the corresponding file
lock held in the browser.

TODO: link blink-dev PSA

Fixed: 1254078, 1380650
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 21, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

Also updates the FSUnderlyingSink to reset its mojo connection when the
stream reaches an errored state. WritableStreams are no longer usable
after an error, so resetting the remote releases the corresponding file
lock held in the browser.

TODO: link blink-dev PSA

Fixed: 1254078, 1380650
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
aarongable pushed a commit to chromium/chromium that referenced this issue Nov 21, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

Also updates the FSUnderlyingSink to reset its mojo connection when the
stream reaches an errored state. WritableStreams are no longer usable
after an error, so resetting the remote releases the corresponding file
lock held in the browser.

blink-dev PSA: https://groups.google.com/a/chromium.org/g/blink-dev/c/ddrh_bI1stY

Fixed: 1254078, 1380650
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3817911
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Daseul Lee <dslee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1074250}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 21, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

Also updates the FSUnderlyingSink to reset its mojo connection when the
stream reaches an errored state. WritableStreams are no longer usable
after an error, so resetting the remote releases the corresponding file
lock held in the browser.

blink-dev PSA: https://groups.google.com/a/chromium.org/g/blink-dev/c/ddrh_bI1stY

Fixed: 1254078, 1380650
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3817911
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Daseul Lee <dslee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1074250}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 21, 2022
removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

Also updates the FSUnderlyingSink to reset its mojo connection when the
stream reaches an errored state. WritableStreams are no longer usable
after an error, so resetting the remote releases the corresponding file
lock held in the browser.

blink-dev PSA: https://groups.google.com/a/chromium.org/g/blink-dev/c/ddrh_bI1stY

Fixed: 1254078, 1380650
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3817911
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Daseul Lee <dslee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1074250}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 11, 2022
…e lock, a=testonly

Automatic update from web-platform-tests
FSA: Make removeEntry() take an exclusive lock

removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

Also updates the FSUnderlyingSink to reset its mojo connection when the
stream reaches an errored state. WritableStreams are no longer usable
after an error, so resetting the remote releases the corresponding file
lock held in the browser.

blink-dev PSA: https://groups.google.com/a/chromium.org/g/blink-dev/c/ddrh_bI1stY

Fixed: 1254078, 1380650
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3817911
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Daseul Lee <dslee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1074250}

--

wpt-commits: 703f1c81703c7014b0515518bc8467870ef8ec0e
wpt-pr: 35407
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Dec 14, 2022
…e lock, a=testonly

Automatic update from web-platform-tests
FSA: Make removeEntry() take an exclusive lock

removeEntry() currently takes a shared lock to allow for removal of a
file with an open writable. Taking an exclusive lock makes the behavior
consistent with move() and remove(), the other file-altering operations.

Discussed in whatwg/fs#39

Also updates the FSUnderlyingSink to reset its mojo connection when the
stream reaches an errored state. WritableStreams are no longer usable
after an error, so resetting the remote releases the corresponding file
lock held in the browser.

blink-dev PSA: https://groups.google.com/a/chromium.org/g/blink-dev/c/ddrh_bI1stY

Fixed: 1254078, 1380650
Change-Id: I5d471405d65f4d1920e7f0dfe9c783a4038e63e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3817911
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Daseul Lee <dslee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1074250}

--

wpt-commits: 703f1c81703c7014b0515518bc8467870ef8ec0e
wpt-pr: 35407
@a-sully
Copy link
Collaborator

a-sully commented Jan 10, 2023

I know that this issue has been inactive for a while now (and that's partly on me - it's quite dependent on #59 and I've been meaning to send an update on that issue for a while) but a WPT was recently updated to assert that createWritable() fails if the file has been removed. The question of "can I delete and then re-create a file from the same handle?" has big implications for the open questions on both this issue and #59). It would be nice to have more discussion on these issues before asserting behaviors in the WPTs... (and I acknowledge I'm likely guilty of doing this, too, so please call me out on that going forward)

One of the key motivating use cases for the remove() of self method is to remove a handle selected via showSaveFilePicker(). From that explainer:

It's quite common for a site to obtain a file handle from showSaveFilePicker(), but then decide not to save after all, and want to delete the file.

Now that remove() is available, it will presumably become common for a site to then decide that they actually do want to save a file. If remove() effectively makes a handle unusable, this is a very frustrating developer experience, since it requires showing yet another file picker to save the file. Developers will be incentivized to avoid using this method outside of the OPFS, resulting a poorer user experience (since the alternative is to leave an empty file).

My understanding of the counter-arguments are:

  1. A reference-based design is easier to implement if you can assume a handle is unusable after it was removed
  2. Deletion + re-creation of directories from the same handle is currently not possible, though that's certainly something we can change

Perhaps we need a more explicit way to re-create a file from a handle other than createWritable()?

@annevk
Copy link
Member

annevk commented Jan 11, 2023

cc @whatwg/fs

@a-sully
Copy link
Collaborator

a-sully commented Jan 11, 2023

To give some other updates on this issue...

I'm going to assume we should take an exclusive lock, and if that fails return InvalidModification

As of web-platform-tests/wpt#35407, removeEntry() takes an exclusive lock on the file (matching the behavior of remove()). Locking errors should always return NoModificationAllowedError, though

Note another option for remove(): if this was unix-posix-like semantics, removing a file with an active reference would not impact the ability to use existing handles to the removed item.

Outside of the OPFS, a multiple sites may have handles corresponding to the same file. This means a site could be prevented from actually deleting a file is another site happens to have a handle, which is confusing and frustrating for developers. Even if we ignore this and only look at the OPFS, I expect developers to expect that remove() deletes the underlying file/directory (which is what #9 specifies)

We could consider adding a flag to remove() to support "unlink" behavior, but I don't think this should be the default

@a-sully
Copy link
Collaborator

a-sully commented Jun 6, 2023

Some more updates on this issue...

This core question of this issue was addressed by #96, which clarified that a FileSystemHandle maps to a file path - and therefore if the underlying file is removed, the FileSystemHandle itself is not changed.

From my perspective, there are still a few open related questions, which I've filed separate issues to discuss:

  1. Do we need a create() self method? See Add a FileSystemHandle.create() method to create self #126
  2. What should createWritable() do if the file does not exist? See Clarify behavior of createWritable() when the corresponding file does not exist #125

Closing this issue for now, but please re-open if there is more to discuss!

@a-sully a-sully closed this as completed Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants