Add Access Handles to spec

[fivedots]

This PR adds Access Handles to the spec. The Access Handles surface differs from existing ones by offering in-place and exclusive write access to a file’s content. This change, along with the ability to consistently read unflushed modifications and the availability of a synchronous variant on dedicated workers, significantly improves performance and unblocks new use cases.

There are still a few open TODOs where advice would be appreciated!

The PR also updates AccessHandles.md to reflect changes in WICG/file-system-access#367.

• At least two implementers are interested (and none opposed):
  • Chrome is in the process of shipping
  • Safari has shipped
• Tests are written and can be reviewed and commented upon at:
  • https://github.com/web-platform-tests/wpt/tree/master/file-system-access
• Implementation bugs are filed:
  • Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1218431
  • Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1761370
  • Safari: https://bugs.webkit.org/show_bug.cgi?id=231185

---

Preview | Diff

[fivedots]

A first pass review was done by @domenic in WICG/file-system-access#344.

[tomayac]

Please consider changing the code samples as in https://github.com/WICG/file-system-access/blob/main/AccessHandle.md, so it's more clear by the variable names what is a FileSystemAccessHandle and what is a FileSystemFileHandle.

// In all contexts
const accessHandle = await fileHandle.createAccessHandle();
await accessHandle.writable.getWriter().write(buffer);
const reader = accessHandle.readable.getReader({ mode: "byob" });
// Assumes seekable streams, and SharedArrayBuffer support are available
await reader.read(buffer, { at: 1 });

// Only in a worker context
const accessHandle = await fileHandle.createSyncAccessHandle();
const writtenBytes = accessHandle.write(buffer);
const readBytes = accessHandle.read(buffer, { at: 1 });

const accessHandle1 = await fileHandle.createAccessHandle();
try {
  const accessHandle2 = await fileHandle.createAccessHandle();
} catch (e) {
  // This catch will always be executed, since there is an open access handle
}
await accessHandle1.close();
// Now a new access handle may be created

[fivedots]

Updated examples on AccessHandles.md as proposed by @tomayac.

[annevk]

I don't think we should keep AccessHandle.md as a separate file. If the information in there is relevant to the standard, it should be folded into the main document. If it's not, it should probably be maintained elsewhere. At least we don't usually have such documents alongside WHATWG standards.

[annevk]

I took a pass at this and mainly found style nits. Hope that helps. Thanks for pushing this forward!

[jesup]

I notice that while the webidl indicates that SyncAccessHandle is for workers only, the commentary for webdevelopers doesn't mention workers anywhere.

I also note that there's no definition of AccessHandles (such as you see in https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems), which means the only way to read data is on worker via SyncAccessHandles (WritableFileStreams only support writing data here). I presume this is an intentional decision made sometime since AcessHandle.md was written? And this means there's no way to read data in an async fashion, it's sync only.

There's also no mention of OPFS until the very end of the document; it's referenced by description in the intro, but not by name.

And ditto to anne's points, especially as to why truncate/getSize are async (since they're only for SyncAccessHandle).

[jesup]

Any plans to resolve this todo in a manner that we can implement (without all of Storage API)? What does chrome's current implementation do, and what does Safari do?

[a-sully]

Not sure what's meant by a "storage shelf" but I can speak to what the current Chrome implementation looks like...

Quota is managed in the browser process, so in order to minimize IPCs we pre-allocate quota. Quota pre-allocations are based on a heuristic which tries to strike a balance between limiting IPCs (for performance) and limiting the amount of quota we're over-allocating (for fairness). Of course one can construct a write pattern such that every write() does an IPC to the browser, but in the average case write() calls shouldn't require an IPC.

Once the AccessHandle is closed, all over-allocated quota is released.

I'm happy to share more specifics, but I assumed quota management would be non-normative? Except for maybe specifying that quota should be accurate when the AccessHandle is closed?

[jesup]

The real missing part here is specifying what happens in quota cases for write() (as is done in WritableFileStream):

If the operations modifying stream.[[buffer]] in the previous steps failed due to exceeding the storage quota, reject p with a QuotaExceededError and abort, leaving stream.[[buffer]] unmodified.

Note: Storage quota only applies to files stored in the origin private file system. However this operation could still fail for other files, for example if the disk being written to runs out of disk space.

[annevk]

(A storage shelf is essentially the storage space for an "origin" (really storage key). https://storage.spec.whatwg.org/#model goes into this in more detail.)

[fivedots]

There is some handling for out-of-quota cases below:

11. If |newSize| − |oldSize| exceeds the available [=storage quota=], throw a {{QuotaExceededError}}

Are there other cases I should handle?

The TODO was more intended as a request for help on to properly spec this behaviour, in particular while using the language that takes the storage bucket/shelf into account.

[jesup]

I see that QuotaExceededError is thrown for write() and for truncate(). Is verbiage needed like "and abort." on these?
If writePosition is larger than oldSize, append writePosition − oldSize 0x00 (NUL) bytes to the end of fileContents. --- this could cause us to exceed quota (and modify the file and then throw, which probably is bad.) We need to check quota before modifying the file.

Creating a file or directory takes space on disk, even if the file is 0 length; if that's considered part of the quota, then those should be able to fail with QuotaExceededError as well. I'm not certain if quota is meant to just be contents, or an approximation of the space required on disk.

My best guess is that quota is just file contents, so this shouldn't be an issue. Double-checking

[fivedots]

I see that QuotaExceededError is thrown for write() and for truncate(). Is verbiage needed like "and abort." on these?

Since these methods don't return a promise, aborting after throwing should not be needed.

If writePosition is larger than oldSize, append writePosition − oldSize 0x00 (NUL) bytes to the end of fileContents. --- this could cause us to exceed quota (and modify the file and then throw, which probably is bad.) We need to check quota before modifying the file.

In this case, the file should not be modified. |fileContents| is a copy of the file's data, the actual file is only modified on step 12 after doing a quota check.

Creating a file or directory takes space on disk, even if the file is 0 length; if that's considered part of the quota, then those should be able to fail with QuotaExceededError as well. I'm not certain if quota is meant to just be contents, or an approximation of the space required on disk.

Those checks (if they are necessary) should have already been done, since to obtain an access handle you need to open/create a file before.

[jesup]

Ok

[a-sully]

Apologies for not looking at this sooner! I've added some notes about Chrome's current implementation, which I hope can serve as a starting point but which of course we can readily change

[jesup]

See this doc for the justification behind the split sync/async interface.

I think you mean this

[a-sully]

See this doc for the justification behind the split sync/async interface.

I think you mean this

Ah yes, sorry about that. I've updated the link in my comment.

[a-sully]

LGTM, but I know @annevk will not be around for a while to address some of the open comment threads... @jesup @tomayac do you think we can just land the PR with TODOs for those questions? We're eager to land this PR so we can put up a follow-up to make the interface all-sync (as proposed in #7), which will make some (but not all) of those questions no longer relevant.

[a-sully]

Friendly ping @jesup @annevk

I'd like to get this PR approved and merged so we can move forward with making all the methods of the SyncAccessHandle sync :)

Thanks!

[jesup]

@a-sully The WPT tests seem to assume you must pass 'at', and if you don't all reads start at 0. This is ... odd, especially for implementing the equivalent to read(), which maintains an internal position in the file, and reads from there. It means for almost all reads (or writes), you must pass at, which means you must track the expected position in the file - likely in a wrapper around syncaccesshandle.read()/write(). Emscriptem certainly can track the location in the file and insert at:nnnn always, but this seems like an odd choice for an API (and not great for performance, due to the extra parsing and validation and the basically forced extra kernel call to seek -- we could avoid the seek() call if we also track the position and elide if if it's a no-op, but that's not worth it either).
I imagine this is legacy decisions from the non-OPFS File System API

[jesup]

Though maybe not legacy -- SyncAccessHandles came later. Can (should?) we change this? If you're writing a bunch of data, it's a whole lot more (unnecessary) bookkeeping and overhead.

   position += handle.write(data1, {at: position});
   position += handle.write(data2, {at: position});

But you can't quite do that, since if there's an error and a partial write, you won't notice it. To be pedantic about catching errors, you need to do:

try {
  written = handle.write(data1, {at: position});
  if (written != data1.length()) { /* error */} else { postion += written;}
  written = handle.write(data2, {at: position});
  if (written != data2.length()) { /* error */} else { postion += written;}
} catch(e) { ... }

With classic posix-like read/write (with an assumed position update), it would be

  handle.write(data1);
  handle.write(data2);

or

try {
  if (handle.write(data1) != data1.length) { /* error */}
  if (handle.write(data2) != data2.length) { /* error */}
} catch(e) { ... }

[jesup]

We should probably merge this and then raise any points like the above as issues

[jesup]

@tomayac Thoughts on at: ?

[a-sully]

We should probably merge this and then raise any points like the above as issues

I agree. Would you mind opening an issue for this? I have some thoughts here and it would be nice to have all the discussion in one place

[jesup]

Editorial nit: in truncate(): "If the offset is smaller than offset, it remains unchanged."

[jesup]

#49

[fivedots]

I also agree merging and moving the discussion to #49 sounds good.

I've updated the description of truncate to fix the confussion mentioned in #21 (comment).

@jesup would you mind approving the PR for completeness/visibility? I'll merge it in then!

[jesup]

I can't approve since I don't have commit access, but go ahead.