Define a parallel queue for all file system operations #95

a-sully · 2023-01-18T01:11:53Z

Fixes #74 by specifying a parallel queue for all file system operations, not just locking (see https://github.com/whatwg/fs/pull/9/files#r1109679577)

At least two implementers are interested (and none opposed):
- Chromium
- WebKit
Tests are written and can be reviewed and commented upon at:
- N/A (unless there's a way to deterministically test race conditions that I don't know about)
Implementation bugs are filed:
- Chromium: ...
- Gecko: …
- WebKit: …
MDN issue is filed: …

Preview | Diff

a-sully · 2023-01-18T01:15:01Z

cc @jesup @szewai

index.bs

annevk

This looks better, but not quite there yet. I'll be out next week, but happy to look at this again in February.

annevk · 2023-01-27T16:24:37Z

index.bs

+       1. Increase |count| by one.
+       1. [=Queue a task=] to |replyTaskSource| to run |onLockTakenAlgorithm|
+          and return.
+  1. [=Reject=] |p| with a "{{NoModificationAllowedError}}" {{DOMException}}.


You have to queue a task for this as well. But maybe it makes more sense that you don't pass the promise in to this algorithm and you have a single callback algorithm that you then pass success/failure to or some such?

Makes sense! The original algorithm returned true or false, but it now runs the passed-in algorithm with "success" and "failure" explicitly, which seems more clear. Let me know if there's a better way to do this!

index.bs

annevk

Mainly lots of nits, but this looks to be heading in the right direction to me!

index.bs

annevk

With some more task queueing I think this can land.

index.bs

a-sully · 2023-02-24T00:13:15Z

Updated to post all file system operations to a "file system queue", as we discussed offline. Lock take/release is once again sync (and must be called from that queue). Please feel free to nit-pick this as much as you please, since this pattern will be applied to all the other methods. Thanks!

annevk

The overall setup here seems sound from a quick skim. The main problem is that "reject", "resolve", and "create a new X" are operations that need to happen on the main thread. And as such you need to do the "queue a storage task" dance for them.

Note that you don't necessarily need to replace each individually. You might be able to restructure things a bit so do all the in parallel work first and safe the results in variables and then queue a storage task that does the rejecting/resolving as per the variables.

Also, what kind of exception can "request access" throw, out of curiosity? Since we get that exception while in parallel, forwarding it probably doesn't entirely work. If we could normalize that to it not returning "granted" that would be better.

index.bs

a-sully · 2023-03-02T00:20:47Z

The overall setup here seems sound from a quick skim. The main problem is that "reject", "resolve", and "create a new X" are operations that need to happen on the main thread. And as such you need to do the "queue a storage task" dance for them.

Oh I see... Question - does this work? If not, why not?

1. [=Queue a storage task=] with [=this=]'s [=relevant global object=] to
   [=enqueue the following steps=] to the [=file system queue=]:
   1. ...

Note that you don't necessarily need to replace each individually. You might be able to restructure things a bit so do all the in parallel work first and safe the results in variables and then queue a storage task that does the rejecting/resolving as per the variables.

Also, what kind of exception can "request access" throw, out of curiosity? Since we get that exception while in parallel, forwarding it probably doesn't entirely work. If we could normalize that to it not returning "granted" that would be better.

Unfortunately I think this is needed, since it can reject with a SecurityError. See https://wicg.github.io/file-system-access/#permissions

annevk · 2023-03-02T13:15:12Z

Question - does this work? If not, why not?

Because you still end up in parallel when doing "resolve", etc. You need some things to happen "in parallel" and then after those things you queue a task for things to happen on the main thread. Such as resolving a promise or creating a JS object.

Unfortunately I think this is needed, since it can reject with a SecurityError.

Hmm, I guess we should redesign this somewhat whereby "request access" returns some kind of internal value and we then later turn that into the appropriate exception.

a-sully · 2023-04-04T04:52:23Z

Question - does this work? If not, why not?

Because you still end up in parallel when doing "resolve", etc. You need some things to happen "in parallel" and then after those things you queue a task for things to happen on the main thread. Such as resolving a promise or creating a JS object.

Ah yes, looking at this with fresh eyes I see the problem. I've updated a few methods - if that pattern looks good, I'll copy it everywhere else

Unfortunately I think this is needed, since it can reject with a SecurityError.

Hmm, I guess we should redesign this somewhat whereby "request access" returns some kind of internal value and we then later turn that into the appropriate exception.

This is a bit tricky because the WICG spec considers file system access a "powerful feature", and powerful features have permission request algorithms that are allowed to throw. I've added an explicit warning that this spec doesn't comply with that pattern, which seems like it should be good enough?

Anyways, I've updated the access checks to return either a PermissionState or an error code that can be turned into a DomException (though I wonder if returning (as opposed to throwing) a DomException directly is an allowed pattern?)

If this all seems reasonable, I'll file an issue to update the WICG spec (and update all the instances in this spec which assume these algorithms can throw)

a-sully · 2023-04-04T04:53:58Z

index.bs

+<p class=warning> Dependent specifications may consider this API a
+[=powerful feature=]. However, unlike other [=powerful features=] whose
+[=permission request algorithm=] may throw, [=/file system entry=]'s
+[=file system entry/query access=] and [=file system entry/request access=]
+algorithms must run [=in parallel=] on the [=file system queue=] and are
+therefore not allowed to throw. Instead, the caller is expected to [=/reject=]
+as appropriate should these algorithms return an [=exception/error name=].


This really should go in WICG spec, but since we're doing something a bit funky it might be useful to have here, too?

If they run in parallel the caller will also be in parallel. They wouldn't be able to immediately reject something.

But also I'm not a big fan of this design. Perhaps these should return a struct?

If they run in parallel the caller will also be in parallel. They wouldn't be able to immediately reject something.

Updated to say the caller is expected to [=queue a storage task=] to [=/reject=], as appropriate

... though while going through the whole spec I realized that there is one situation - the asynchronous iterator initialization steps - where there is no promise to reject. See the other comment

But also I'm not a big fan of this design. Perhaps these should return a struct?

Done. AFAICT a struct item cannot be "optional" (only algorithm parameters?) so "error name" is just empty most of the time

index.bs

a-sully · 2023-05-10T23:33:15Z

if that pattern looks good, I'll copy it everywhere else

friendly ping @annevk. This infra PR is holding up the PRs specifying move() and remove()

annevk

Thanks for your patience. This is looking rather good. Except the access checks, those could do with some more design work.

annevk · 2023-05-31T11:04:03Z

index.bs

+<p class=warning> Dependent specifications may consider this API a
+[=powerful feature=]. However, unlike other [=powerful features=] whose
+[=permission request algorithm=] may throw, [=/file system entry=]'s
+[=file system entry/query access=] and [=file system entry/request access=]
+algorithms must run [=in parallel=] on the [=file system queue=] and are
+therefore not allowed to throw. Instead, the caller is expected to [=/reject=]
+as appropriate should these algorithms return an [=exception/error name=].


If they run in parallel the caller will also be in parallel. They wouldn't be able to immediately reject something.

But also I'm not a big fan of this design. Perhaps these should return a struct?

index.bs

Fixes whatwg#74

a-sully

As promised, I've updated the whole spec to follow the pattern. Note that much of the diff is just formatting (e.g. adding an indentation)

I've manually double-checked that all promise rejections and resolutions now happen from the "storage task source" and all access checks are done in-parallel from the file system queue (though it's still possible I've missed something...)

index.bs

a-sully · 2023-06-02T05:07:40Z

index.bs

+<p class=warning> Dependent specifications may consider this API a
+[=powerful feature=]. However, unlike other [=powerful features=] whose
+[=permission request algorithm=] may throw, [=/file system entry=]'s
+[=file system entry/query access=] and [=file system entry/request access=]
+algorithms must run [=in parallel=] on the [=file system queue=] and are
+therefore not allowed to throw. Instead, the caller is expected to [=/reject=]
+as appropriate should these algorithms return an [=exception/error name=].


If they run in parallel the caller will also be in parallel. They wouldn't be able to immediately reject something.

Updated to say the caller is expected to [=queue a storage task=] to [=/reject=], as appropriate

... though while going through the whole spec I realized that there is one situation - the asynchronous iterator initialization steps - where there is no promise to reject. See the other comment

But also I'm not a big fan of this design. Perhaps these should return a struct?

Done. AFAICT a struct item cannot be "optional" (only algorithm parameters?) so "error name" is just empty most of the time

a-sully · 2023-06-02T05:13:13Z

index.bs

+    1. Set |accessErrorName| to |accessResult|'s
+       [=file system access result/error name=] if it is not
+       « the empty string »; otherwise "{{NotAllowedError}}".
+    1. [=Throw=] an |accessErrorName| {{DOMException}}.


We've specified that query access requires going in parallel on the file system queue, but if we get an exception there's no associated promise to reject...

... @mkruisselbrink do you know if this is this even necessary? Or is it enough to check query access in just the "get the next iteration result" algorithm below?

It doesn't seem to do anything based on these steps. Per https://webidl.spec.whatwg.org/#idl-async-iterable we can omit it I think.

cc @domenic

Tentatively omitted it for now, pending input from others

a-sully · 2023-06-02T05:14:42Z

index.bs

+      1. [=/Reject=] |promise| with an |accessErrorName| {{DOMException}}
+         and return |promise|.
+
+    1. If |directory| is `null`, [=/reject=] |result| with a
+       "{{NotFoundError}}" {{DOMException}} and abort these steps.


Note: this is the only "behavioral" change in this PR. Presumably the "NotAllowedError" should take precedence over the "NotAllowedError", just like everywhere else

a-sully · 2023-06-02T05:19:37Z

index.bs

@@ -135,7 +176,7 @@ A <dfn export id=directory>directory entry</dfn> additionally consists of a [=/s
 <dfn for="directory entry">children</dfn>, which are themselves [=/file system entries=].
 Each member is either a [=/file entry=] or a [=/directory entry=].

-A [=/file system entry=] |entry| should be [=list/contained=] in the [=children=] of at most one
+A [=/file system entry=] |entry| should be [=list/contained=] in the [=directory entry/children=] of at most one


FYI: this fixes a spec build failure

index.bs

annevk · 2023-06-02T12:18:22Z

index.bs

+:: A [=string=] which must be either « the empty string » or an
+   [=exception/error name=] listed in the [=error names table=].
+   If [=file system access result/permission state=] is
+   "{{PermissionState/granted}}" this must be « the empty string »


Can we make it so that when permission is denied it has to be an error name so we don't have to do defaulting below?

Done. This simplifies things quite a lot

Side note: is "iff" okay to use in specs? Happy to spell it out if not

annevk · 2023-06-02T12:25:02Z

index.bs

+    1. Set |accessErrorName| to |accessResult|'s
+       [=file system access result/error name=] if it is not
+       « the empty string »; otherwise "{{NotAllowedError}}".
+    1. [=Throw=] an |accessErrorName| {{DOMException}}.


It doesn't seem to do anything based on these steps. Per https://webidl.spec.whatwg.org/#idl-async-iterable we can omit it I think.

cc @domenic

a-sully · 2023-06-07T13:39:10Z

@annevk friendly ping (this is holding up a number of other changes). If this looks good, feel free to merge

annevk

Found a couple remaining nits, generally looks good. Feel free to merge after addressing them. Though I should have time tomorrow if you'd rather I do it.

index.bs

annevk

I meant to approve this to make that possible. Doh.

prompted by whatwg/webidl@fcbaa09

a-sully · 2023-06-13T16:37:32Z

Thanks for the review! I'll merge this now so we can start making progress on the various PRs that are blocked on it. If you have more nits, I'm happy to address them in a follow-up

Just an FYI that I also updated the permission checks to use the new [=DOMException/name=] and "names table" added in whatwg/webidl#1211 (which broke the refs we were adding)

a-sully requested a review from annevk January 18, 2023 01:15

a-sully mentioned this pull request Jan 18, 2023

Add FileSystemHandle.remove method #9

Open

3 tasks

annevk reviewed Jan 24, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

a-sully force-pushed the fix-74 branch from 3788a84 to dfc6d46 Compare January 25, 2023 23:06

annevk reviewed Jan 27, 2023

View reviewed changes

a-sully commented Jan 28, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

annevk reviewed Feb 14, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

a-sully added a commit to a-sully/fs that referenced this pull request Feb 15, 2023

use framework laid out in whatwg#95

aa0b505

annevk reviewed Feb 15, 2023

View reviewed changes

a-sully changed the title ~~Define a parallel queue for lock operations~~ Define a parallel queue for all file system operations Feb 24, 2023

annevk reviewed Feb 24, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

a-sully mentioned this pull request Mar 1, 2023

Give FileSystemHandles an associated path #96

Merged

4 tasks

a-sully mentioned this pull request Apr 4, 2023

Moving local files with the File System Access API w3ctag/design-reviews#805

Closed

1 task

a-sully force-pushed the fix-74 branch 2 times, most recently from ea97146 to aff3e07 Compare April 4, 2023 04:39

a-sully commented Apr 4, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

a-sully mentioned this pull request Apr 6, 2023

Specify "origin private file system"-ness on a FileSystemHandle #110

Merged

annevk reviewed May 31, 2023

View reviewed changes

a-sully added 5 commits May 31, 2023 16:15

Define a parallel queue for lock operations

4d3958a

Fixes whatwg#74

nit

3844605

run task if lock taken - sketch

7d63797

pass only one algorithm

3bb5521

wait for lock release on close

346b7d6

a-sully added 6 commits May 31, 2023 16:15

queue a storage task

f727e37

simplify lock-taking in createSAH

9694ad1

use parallel queue for all FS operations

0c515c2

queue a task to enqueue steps

334d4e6

rebase + make access checks not throwable

f2d6b04

apply pattern everywhere

423d09d

a-sully force-pushed the fix-74 branch from aff3e07 to 423d09d Compare June 2, 2023 05:00

update warning

c20e4bd

a-sully commented Jun 2, 2023

View reviewed changes

annevk reviewed Jun 2, 2023

View reviewed changes

assume error name is returned if not granted

5a7d095

annevk reviewed Jun 13, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

index.bs Outdated Show resolved Hide resolved

annevk approved these changes Jun 13, 2023

View reviewed changes

a-sully added 2 commits June 13, 2023 12:21

address nits

a189311

update error name ref

ba5a295

prompted by whatwg/webidl@fcbaa09

a-sully merged commit 2a7b613 into whatwg:main Jun 13, 2023

a-sully deleted the fix-74 branch June 13, 2023 16:49

a-sully mentioned this pull request Jun 14, 2023

Editorial: Don't use dot access in most places #90

Merged

This was referenced Jun 22, 2023

Use the new Storage Task Source and File System Queue WICG/file-system-access#420

Open

Queue tasks #3

Closed

a-sully added a commit to a-sully/fs that referenced this pull request Jun 22, 2023

retry specifying remove after whatwg#95

957ffea

This was referenced Jul 11, 2023

Support locking of directory entries #139

Open

Add FileSystemHandle.move method #10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define a parallel queue for all file system operations #95

Define a parallel queue for all file system operations #95

a-sully commented Jan 18, 2023 •

edited by pr-preview bot

Loading

a-sully commented Jan 18, 2023

annevk left a comment

annevk Jan 27, 2023

a-sully Jan 28, 2023

annevk left a comment

annevk left a comment

a-sully commented Feb 24, 2023

annevk left a comment

a-sully commented Mar 2, 2023

annevk commented Mar 2, 2023

a-sully commented Apr 4, 2023

a-sully Apr 4, 2023

annevk May 31, 2023

a-sully Jun 2, 2023

a-sully commented May 10, 2023

annevk left a comment

annevk May 31, 2023

a-sully left a comment

a-sully Jun 2, 2023

a-sully Jun 2, 2023

annevk Jun 2, 2023

a-sully Jun 2, 2023

a-sully Jun 2, 2023

a-sully Jun 2, 2023

annevk Jun 2, 2023

a-sully Jun 2, 2023

annevk Jun 2, 2023

a-sully commented Jun 7, 2023

annevk left a comment

annevk left a comment

a-sully commented Jun 13, 2023

Define a parallel queue for all file system operations #95

Define a parallel queue for all file system operations #95

Conversation

a-sully commented Jan 18, 2023 • edited by pr-preview bot Loading

a-sully commented Jan 18, 2023

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

a-sully commented Feb 24, 2023

annevk left a comment

Choose a reason for hiding this comment

a-sully commented Mar 2, 2023

annevk commented Mar 2, 2023

a-sully commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-sully commented May 10, 2023

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-sully left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-sully commented Jun 7, 2023

annevk left a comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

a-sully commented Jun 13, 2023

a-sully commented Jan 18, 2023 •

edited by pr-preview bot

Loading