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

Possible options in FileSystemFileHandleCreateAccessHandleOptions #19

Closed
a-sully opened this issue Mar 9, 2022 · 4 comments
Closed

Comments

@a-sully
Copy link
Collaborator

a-sully commented Mar 9, 2022

Migrated from WICG/file-system-access#326 (see #2)

@mkruisselbrink

I've been thinking a bit about what options might make sense to be able to pass to the createAccessHandle and createSyncAccessHandle methods, primarily to make sure we won't paint ourselves into a corner if we initially ship only one "flavor" of access handles (i.e. exclusively locked, in-place writable access handles), as currently described in the AccessHandle proposal.

I'm currently thinking there might be three separate axes on which it could make sense to change the behavior of access handles:

File locking behavior (don't lock the file at all, hold a shared (aka read) lock on the file, or hold an exclusive lock on the file)
Should the handle operate on the file in-place, or atomically on a swap/temporary version of the file (either starting out blank or created by copying the previously existing file)
Should the handle provide read-only access or allow both reading and writing.
I'm not sure if all possible combinations of these make sense, but I think quite a few of them probably do. For example:

We would only allow in-place, readwrite access for files in the origin private file system, for the same reasons we don't have an in-place mode for createWritable today (on the other hand, in-place read-only mode should be just fine for arbitrary file handles).

We probably want to disallow non-locking or shared-locking in-place read-write access, as that would enable changes from one frame to be immediately visible in other frames, resulting in potentially high-frequency timers etc. Requiring a page to first obtain a exclusive lock before being able to write in-place would give us the opportunity to make sure this can't be exploited as a super-low-latency communication mechanism.

One question I'm not sure about is if we'd also want to enable a particular AccessHandle to transition between different locking modes. Is there benefit to being able to upgrade a shared lock to an exclusive lock without having to re-open the access handle (or the reverse, drop from exclusive down to shared/no-lock)? My hope would be that we wouldn't need that, but I'm not an expert in how people use file locking.

Another question is what locking a file actually means for files outside the origin private file system. Enforcing the same locking semantics for other access via the File System Access API in the same browser instance would be a start, but probably we would also want to lock the actual files on disk with whatever file locking mechanism the underlying OS supports. Afaik on posix file locks are advisory only, but on windows shared and exclusive file locks do exist. I think we should aim for a best-effort do whatever is typical on the underlying FS, while enforcing well specified semantics for other access using the File System Access API.

Finally, some strawman IDL proposals:

enum FileSystemLockMode { "none", "shared", "exclusive", };
enum FileSystemAccessMode { "read", "readwrite", };
enum FileSystemWriteMode {"atomic", "atomic-from-copy", "in-place"};

dictionary FileSystemFileHandleCreateAccessHandleOptions {
  FileSystemAccessMode access = "readwrite";
  FileSystemWriteMode mode = "atomic";
  FileSystemLockMode lock;
};

I.e. the default would be similar to what createWritable does today, while if you want the current Access Handle proposal's behavior you'd have to pass {mode: "in-place"}. Not entirely sure what the default for lock should be. For in-place readwrite mode "exclusive" is the only valid option, so that should probably be it. On the other hand for read-only access it might make more sense to default to "shared" if no lock mode is provided.

In an initial implementation of the current Access Handle proposal in chrome we would implement this as

enum FileSystemWriteMode {"in-place"};

dictionary FileSystemFileHandleCreateAccessHandleOptions {
  required FileSystemWriteMode mode;
};

which more or less matches the current Access Handle proposal anyway (the only difference with the current proposal being that mode would be required), while this can relatively easily be backwards compatibly extended to cover the larger feature set described above.

@tomayac

Super nit remark: I know where we come from with this, but the below…

enum FileSystemAccessMode { "read", "readwrite", };

…is the odd one out that’s spelled without a dash ‘-‘.

@a-sully
Copy link
Collaborator Author

a-sully commented Mar 9, 2022

Relatedly, we should consider adding an autoClose flag to FileSystemCreateWritableOptions as suggested in WICG/file-system-access#236

@mkruisselbrink

Currently you need to explicitly close a FileSystemWritableFileStream for the changes written to it to be flushed to disk. Usually this is a good thing, since it means that a website won't accidentally overwrite a file with a partial version of the file if for whatever reason the website is closed while it is writing to a file. I.e. changes are written to a temporary file and then atomically moved into place.

Sometimes websites to want to be able to write partial data to files though. For example in the case of an audio or video recorder it might be totally reasonable to just want to write however much data could be written, even if the app is closed unexpectedly.

To support that I propose adding a autoClose flag to FileSystemCreateWritableOptions. By default this is false, keeping the existing behavior. But if it is set to true, changes will be written to the target file even if the stream isn't explicitly closed (when the stream gets garbage collected/the page owning it gets closed). A website can still abort the write by calling abort() on the stream, and websites should still call close() to make sure the data gets moved into place as soon as feasible (relying on garbage collection would it make it very unpredictable when the changes show up on disk).

There is some overlap between this and earlier ideas for an "in-place" mode, but in-place mode makes things like safe browsing and other security checks much harder. So with no clear way forward for in-place, this will at least cover some of its use cases.

@fivedots
Copy link
Contributor

fivedots commented Apr 4, 2022

After conversations with Safari and the Chrome Storage team, we are changing the approach to providing options to create[Sync]AccessHandle() in the current Chromium implementation, as well as in the explainer. To avoid compatibility issues and leave the design of the options object fully open, the parameter will be temporarily removed.

Once there is consensus, the options can be re-added without much issue, e.g.: by considering the current OPFS behaviour to be the default, and requiring the paremeter when this would not be appropriate (like when creating it in a non-OPFS file system).

@annevk
Copy link
Member

annevk commented Apr 5, 2022

That sounds reasonable, but could you keep the Mozilla storage team in the loop as well going forward?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 5, 2022
This reverts commit ca2d8b837fca8a7bd3bd0ba1562c7cc090108374.

Reason for revert: To avoid compatibility issues with Safari (that has already shipped this API without the options param) and leave the design of the object fully open, we decided to remove the parameter. Further context in whatwg/fs#19.

Original change's description:
> Add options parameter to createSyncAccessHandle()
>
> For now the only accepted (and required) option is "mode: 'in-place'".
> Having this parameter allows us to later define a more appropriate
> default behavior for access handles than the one we currently implement.
> This is specially useful in the context of expanding access handle usage
> outside of OPFS.
>
> For more details check:
> https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems
>
> Bug: 1218431
> Change-Id: I50d0a4acfd81874e17b6b28d1c2bf446b0397a4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3110487
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Emanuel Krivoy <krivoy@google.com>
> Cr-Commit-Position: refs/heads/main@{#982209}

Bug: 1218431
Change-Id: I11dd5044b59e1fec8444242869628129efa5cf17
fivedots added a commit to WICG/file-system-access that referenced this issue Apr 5, 2022
In order to prevent caused compatibility issues with WebKit, the options
parameter should be removed from create[Sync]AccessHandle(). For more
context see whatwg/fs#19.
aarongable pushed a commit to chromium/chromium that referenced this issue Apr 6, 2022
This reverts commit ca2d8b8.

Reason for revert: To avoid compatibility issues with Safari (that has already shipped this API without the options param) and leave the design of the object fully open, we decided to remove the parameter. Further context in whatwg/fs#19.

Original change's description:
> Add options parameter to createSyncAccessHandle()
>
> For now the only accepted (and required) option is "mode: 'in-place'".
> Having this parameter allows us to later define a more appropriate
> default behavior for access handles than the one we currently implement.
> This is specially useful in the context of expanding access handle usage
> outside of OPFS.
>
> For more details check:
> https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems
>
> Bug: 1218431
> Change-Id: I50d0a4acfd81874e17b6b28d1c2bf446b0397a4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3110487
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Emanuel Krivoy <krivoy@google.com>
> Cr-Commit-Position: refs/heads/main@{#982209}

Bug: 1218431
Change-Id: I11dd5044b59e1fec8444242869628129efa5cf17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572141
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Emanuel Krivoy <krivoy@google.com>
Cr-Commit-Position: refs/heads/main@{#989350}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 6, 2022
This reverts commit ca2d8b837fca8a7bd3bd0ba1562c7cc090108374.

Reason for revert: To avoid compatibility issues with Safari (that has already shipped this API without the options param) and leave the design of the object fully open, we decided to remove the parameter. Further context in whatwg/fs#19.

Original change's description:
> Add options parameter to createSyncAccessHandle()
>
> For now the only accepted (and required) option is "mode: 'in-place'".
> Having this parameter allows us to later define a more appropriate
> default behavior for access handles than the one we currently implement.
> This is specially useful in the context of expanding access handle usage
> outside of OPFS.
>
> For more details check:
> https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems
>
> Bug: 1218431
> Change-Id: I50d0a4acfd81874e17b6b28d1c2bf446b0397a4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3110487
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Emanuel Krivoy <krivoy@google.com>
> Cr-Commit-Position: refs/heads/main@{#982209}

Bug: 1218431
Change-Id: I11dd5044b59e1fec8444242869628129efa5cf17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572141
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Emanuel Krivoy <krivoy@google.com>
Cr-Commit-Position: refs/heads/main@{#989350}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 6, 2022
This reverts commit ca2d8b837fca8a7bd3bd0ba1562c7cc090108374.

Reason for revert: To avoid compatibility issues with Safari (that has already shipped this API without the options param) and leave the design of the object fully open, we decided to remove the parameter. Further context in whatwg/fs#19.

Original change's description:
> Add options parameter to createSyncAccessHandle()
>
> For now the only accepted (and required) option is "mode: 'in-place'".
> Having this parameter allows us to later define a more appropriate
> default behavior for access handles than the one we currently implement.
> This is specially useful in the context of expanding access handle usage
> outside of OPFS.
>
> For more details check:
> https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems
>
> Bug: 1218431
> Change-Id: I50d0a4acfd81874e17b6b28d1c2bf446b0397a4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3110487
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Emanuel Krivoy <krivoy@google.com>
> Cr-Commit-Position: refs/heads/main@{#982209}

Bug: 1218431
Change-Id: I11dd5044b59e1fec8444242869628129efa5cf17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572141
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Emanuel Krivoy <krivoy@google.com>
Cr-Commit-Position: refs/heads/main@{#989350}
fivedots added a commit to WICG/file-system-access that referenced this issue Apr 7, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 11, 2022
…yncAccessHandle()", a=testonly

Automatic update from web-platform-tests
Revert "Add options parameter to createSyncAccessHandle()"

This reverts commit ca2d8b837fca8a7bd3bd0ba1562c7cc090108374.

Reason for revert: To avoid compatibility issues with Safari (that has already shipped this API without the options param) and leave the design of the object fully open, we decided to remove the parameter. Further context in whatwg/fs#19.

Original change's description:
> Add options parameter to createSyncAccessHandle()
>
> For now the only accepted (and required) option is "mode: 'in-place'".
> Having this parameter allows us to later define a more appropriate
> default behavior for access handles than the one we currently implement.
> This is specially useful in the context of expanding access handle usage
> outside of OPFS.
>
> For more details check:
> https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems
>
> Bug: 1218431
> Change-Id: I50d0a4acfd81874e17b6b28d1c2bf446b0397a4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3110487
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Emanuel Krivoy <krivoy@google.com>
> Cr-Commit-Position: refs/heads/main@{#982209}

Bug: 1218431
Change-Id: I11dd5044b59e1fec8444242869628129efa5cf17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572141
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Emanuel Krivoy <krivoy@google.com>
Cr-Commit-Position: refs/heads/main@{#989350}

--

wpt-commits: 9082234ee4b01d20a4e9eec0297155e2e5f6838f
wpt-pr: 33516
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Apr 14, 2022
…yncAccessHandle()", a=testonly

Automatic update from web-platform-tests
Revert "Add options parameter to createSyncAccessHandle()"

This reverts commit ca2d8b837fca8a7bd3bd0ba1562c7cc090108374.

Reason for revert: To avoid compatibility issues with Safari (that has already shipped this API without the options param) and leave the design of the object fully open, we decided to remove the parameter. Further context in whatwg/fs#19.

Original change's description:
> Add options parameter to createSyncAccessHandle()
>
> For now the only accepted (and required) option is "mode: 'in-place'".
> Having this parameter allows us to later define a more appropriate
> default behavior for access handles than the one we currently implement.
> This is specially useful in the context of expanding access handle usage
> outside of OPFS.
>
> For more details check:
> https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems
>
> Bug: 1218431
> Change-Id: I50d0a4acfd81874e17b6b28d1c2bf446b0397a4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3110487
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Emanuel Krivoy <krivoy@google.com>
> Cr-Commit-Position: refs/heads/main@{#982209}

Bug: 1218431
Change-Id: I11dd5044b59e1fec8444242869628129efa5cf17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572141
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Emanuel Krivoy <krivoy@google.com>
Cr-Commit-Position: refs/heads/main@{#989350}

--

wpt-commits: 9082234ee4b01d20a4e9eec0297155e2e5f6838f
wpt-pr: 33516
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This reverts commit ca2d8b837fca8a7bd3bd0ba1562c7cc090108374.

Reason for revert: To avoid compatibility issues with Safari (that has already shipped this API without the options param) and leave the design of the object fully open, we decided to remove the parameter. Further context in whatwg/fs#19.

Original change's description:
> Add options parameter to createSyncAccessHandle()
>
> For now the only accepted (and required) option is "mode: 'in-place'".
> Having this parameter allows us to later define a more appropriate
> default behavior for access handles than the one we currently implement.
> This is specially useful in the context of expanding access handle usage
> outside of OPFS.
>
> For more details check:
> https://github.com/WICG/file-system-access/blob/main/AccessHandle.md#exposing-accesshandles-on-all-filesystems
>
> Bug: 1218431
> Change-Id: I50d0a4acfd81874e17b6b28d1c2bf446b0397a4d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3110487
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Emanuel Krivoy <krivoy@google.com>
> Cr-Commit-Position: refs/heads/main@{#982209}

Bug: 1218431
Change-Id: I11dd5044b59e1fec8444242869628129efa5cf17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572141
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Emanuel Krivoy <krivoy@google.com>
Cr-Commit-Position: refs/heads/main@{#989350}
NOKEYCHECK=True
GitOrigin-RevId: a299c35d764d6c31ed08f37dd05e92e8838a65b9
@a-sully
Copy link
Collaborator Author

a-sully commented Jan 6, 2023

Closing in favor of #34, where discussions related to this idea are currently taking place

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