Closed Bug 1824305 Opened 1 year ago Closed 10 months ago

FS: Implement temporary files for FileSystemWritableFileStream

Categories

(Core :: DOM: File, task, P3)

task

Tracking

()

RESOLVED FIXED
116 Branch
Tracking Status
firefox116 --- fixed

People

(Reporter: jjalkanen, Assigned: jjalkanen)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(25 files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

According to fs spec, a FileSystemWritableFileStream has an associated buffer. This buffer can get arbitrarily large, so it is expected that implementations will not keep this in memory, but instead use a temporary file for this. All access to buffer is done in promise returning methods and algorithms, so even though operations on it seem sync, implementations can implement them async.

Any changes made through stream won’t be reflected in the file entry locatable by fileHandle’s locator until the stream has been closed. User agents try to ensure that no partial writes happen, i.e. the file will either contain its old contents or it will contain whatever data was written through stream up until the stream has been closed.

This is typically implemented by writing data to a temporary file, and only replacing the file entry locatable by fileHandle’s locator with the temporary file when the writable filestream is closed.

If keepExistingData is false or not specified, the temporary file starts out empty, otherwise the existing file is first copied to this temporary file.

Creating a FileSystemWritableFileStream takes a shared lock on the file entry locatable with fileHandle’s locator. This prevents the creation of FileSystemSyncAccessHandles for the entry, until the stream is closed.

Assignee: nobody → jjalkanen

With the current system, multiple tabs can edit simultaneously by creating their own temporary files to uniquely named folders, and then merge the outcomes deterministically with moves and renames.

FileSystemWritableStreams clearly removes the need for the auxiliary folder system for this use case but merging on close is more subtle.

When the whole browser or just a tab becomes closed, the underlying writables also become closed in a certain sense, but merging the content automatically could be considered as data corruption under many circumstances, such as a crash, misclick, closing the browser while doing something else, browser update or an OS crash. It would probably make sense to distinguish the case when the user explicitly requests for closing the writable, and to avoid the merge in every other situation.

The sync access handle doesn't have this problem due to its exclusive locks.

A potential implementation could ...

  • have the parent process open a new quota tracked regular file, which is associated to the main file by some metadata information, for every writable request
  • cache the association metadata in the tab state by the client
  • retrieve the association metadata from the tab state when a tab is recovered or restored, and let the user continue working with the same OPFS file
  • use an internal close method to close the temporary file without performing a merge
  • replace the main file by the parent process only when the close method is explicitly called from the FileSystemWritableFileStream interface

This would also avoid the complication of merging the files when the content process is shutting down, unless the application wants it.

Quota tracking would work just like for any group of files, except that the main file's contribution would get removed at every merge.

The existing rules of evicting recovery data should probably apply to the cached information. On the OPFS parent process side, the oldest unaccessed temporary files could probably be removed as part of the maintenance, with some reasonable eviction rule, or with more effort, the removal could be coupled to the tab session history.

Flags: needinfo?(rjesup)
Flags: needinfo?(jvarga)
Flags: needinfo?(jstutte)
Flags: needinfo?(hsingh)
Flags: needinfo?(bugmail)

On the quota and metadata aspects I am not competent, but

replace the main file by the parent process only when the close method is explicitly called from the FileSystemWritableFileStream interface

sounds right to me. We might need to be prepared (with locks or a serializing task queue in the parent?) to receive two or more concurrent requests to do so at the same time, however, and the last one to be executed should probably just silently win. Imagine:

Tab A opens File X for write, obtaining X1.
Tab B opens File X for write, obtaining X2.
Tab A closes X1 -> X
While that is in flight, Tab B closes X2 -> X
While that is in flight, Tab A tries to open X again

Should Tab A see X1 or X2 on the second open? Probably all operations that directly involve X (be it open/copy or close) must be serialized in the parent process in order to have at least a defined order and state, and this would result in Tab A seeing X2 in the above example.

Flags: needinfo?(jstutte)
Flags: needinfo?(smaug)

(In reply to Jens Stutte [:jstutte] from comment #2)

Tab A opens File X for write, obtaining X1.
Tab B opens File X for write, obtaining X2.
Tab A closes X1 -> X
While that is in flight, Tab B closes X2 -> X
While that is in flight, Tab A tries to open X again

Should Tab A see X1 or X2 on the second open? Probably all operations that directly involve X (be it open/copy or close) must be serialized in the parent process in order to have at least a defined order and state, and this would result in Tab A seeing X2 in the above example.

So probably we should execute the close and get requests sequentially and

  • if a close request from B with a timestamp 1001 arrives first, and a close request from A arrives second with a timestamp 1000, we would drop the request from A, and in general only keep the highest low-precision timestamp
  • if both requests have the same low-precision timestamp, we would pick the one with a higher (or lower) shared lock id
  • closing is fast as we only need to update where the main file entry points to, while the get requests are slow as a file might need to be copied
  • probably copying the main file should be done asynchronously, and interrupted and restarted every time a new close request arrives

(In reply to Jari Jalkanen from comment #3)

So probably we should execute the close and get requests sequentially and

  • if a close request from B with a timestamp 1001 arrives first, and a close request from A arrives second with a timestamp 1000, we would drop the request from A, and in general only keep the highest low-precision timestamp
  • if both requests have the same low-precision timestamp, we would pick the one with a higher (or lower) shared lock id
  • closing is fast as we only need to update where the main file entry points to, while the get requests are slow as a file might need to be copied
  • probably copying the main file should be done asynchronously, and interrupted and restarted every time a new close request arrives

That sounds like somewhat more complex than just having a serializing event target on the parent side for the file. I would expect that to handle the timestamp part just as well (it always remains a random order between two child processes) and it would help to avoid the restart complexity (at the cost of making the requests wait longer in some cases, probably), IIUC. But I might overlook other implementation details, of course.

If we allow random ordering, or send obsolete copies to the content process, the latest changes sometimes get overwritten. These issues would be easier to avoid without a queue. However, the procedure could probably be implemented as an event target.

Fortunately, we have a per origin FileSystemDataManager with a task queue and this manager is shared by all IPC actors created for windows and workers in content processes. All parent actors for given origin are bound to the task queue owned by the manager. So this provides a good base for the serialization already. So I don't think we need any timestamps or such.

(In reply to Jens Stutte [:jstutte] from comment #2)

On the quota and metadata aspects I am not competent, but

replace the main file by the parent process only when the close method is explicitly called from the FileSystemWritableFileStream interface

sounds right to me. We might need to be prepared (with locks or a serializing task queue in the parent?) to receive two or more concurrent requests to do so at the same time, however, and the last one to be executed should probably just silently win. Imagine:

The initial implementation should just process all explicit close calls and replace the main file with the temporary file (exact technique can be different) one by one. We can later do some optimizations, like aborting an already running replacement of the main file when a new close request arrives.

Tab A opens File X for write, obtaining X1.
Tab B opens File X for write, obtaining X2.
Tab A closes X1 -> X
While that is in flight, Tab B closes X2 -> X
While that is in flight, Tab A tries to open X again

Should Tab A see X1 or X2 on the second open? Probably all operations that directly involve X (be it open/copy or close) must be serialized in the parent process in order to have at least a defined order and state, and this would result in Tab A seeing X2 in the above example.

Yes, we need to serialize it and we already have a task queue in the parent used by actors (as I already mentioned in my previous comment).

(In reply to Jan Varga [:janv] from comment #7)

The initial implementation should just process all explicit close calls and replace the main file with the temporary file (exact technique can be different) one by one. We can later do some optimizations, like aborting an already running replacement of the main file when a new close request arrives.

BTW, for something like this, we need to introduce dedicated task queues for writable file streams, like we did for sync access handles. See bug 1809043 and bug 1809064. The task queue will be also useful for the case when the temporary file needs to be filled with the content of the main file.

(In reply to Jari Jalkanen from comment #0)

According to fs spec, a FileSystemWritableFileStream has an associated buffer. This buffer can get arbitrarily large, so it is expected that implementations will not keep this in memory, but instead use a temporary file for this. All access to buffer is done in promise returning methods and algorithms, so even though operations on it seem sync, implementations can implement them async.

Any changes made through stream won’t be reflected in the file entry locatable by fileHandle’s locator until the stream has been closed. User agents try to ensure that no partial writes happen, i.e. the file will either contain its old contents or it will contain whatever data was written through stream up until the stream has been closed.

This is typically implemented by writing data to a temporary file, and only replacing the file entry locatable by fileHandle’s locator with the temporary file when the writable filestream is closed.

If keepExistingData is false or not specified, the temporary file starts out empty, otherwise the existing file is first copied to this temporary file.

Creating a FileSystemWritableFileStream takes a shared lock on the file entry locatable with fileHandle’s locator. This prevents the creation of FileSystemSyncAccessHandles for the entry, until the stream is closed.

I would also add that the spec mentions in the closeAlgorithm section "Note: It is expected that this atomically updates the contents of the file on disk being written to." before release the lock.

(In reply to Jari Jalkanen from comment #1)

With the current system, multiple tabs can edit simultaneously by creating their own temporary files to uniquely named folders, and then merge the outcomes deterministically with moves and renames.

I think it would be better to use "multiple contexts" terminology, there can be multiple windows and multiple workers for the same tab.

FileSystemWritableStreams clearly removes the need for the auxiliary folder system for this use case but merging on close is more subtle.

I'm not sure I understand the need for uniquely named folders, but I guess you are saying that we actually don't need it ?

When the whole browser or just a tab becomes closed, the underlying writables also become closed in a certain sense, but merging the content automatically could be considered as data corruption under many circumstances, such as a crash, misclick, closing the browser while doing something else, browser update or an OS crash. It would probably make sense to distinguish the case when the user explicitly requests for closing the writable, and to avoid the merge in every other situation.

Yes, and it seems implementing the abortAlgorithm is exactly what we want.

The sync access handle doesn't have this problem due to its exclusive locks.

Well, we probably want something like abort (besides close) even for sync access handles. But obviously, it can be done separately as an optimization for shutdown.

A potential implementation could ...

  • have the parent process open a new quota tracked regular file, which is associated to the main file by some metadata information, for every writable request

Yes, the association can be done easily because we already created sub protocols for syns access handles and writable file streams, so the extra state can be stored in the sub actors.

  • cache the association metadata in the tab state by the client

Not sure what you mean here and why we would need something like this. And again, "tab" is not very suitable for keeping any state because the FS spec or any other related spec doesn't mention something like this AFAIK.

  • retrieve the association metadata from the tab state when a tab is recovered or restored, and let the user continue working with the same OPFS file

Are we talking about the Back/Forward cache here ?

  • use an internal close method to close the temporary file without performing a merge

This would be an abort method used by the abortAlgorithm and we would use it in other places as well, like handling not yet closed handles during shutdown

  • replace the main file by the parent process only when the close method is explicitly called from the FileSystemWritableFileStream interface

Yes

This would also avoid the complication of merging the files when the content process is shutting down, unless the application wants it.

Quota tracking would work just like for any group of files, except that the main file's contribution would get removed at every merge.

Yes, this should be easy, because usage would be increased for the temporary file first (besides keeping usage tracked for the main file) and then we would just decrease the usage by the size of the main file. This should always succeed in quota manager.

The existing rules of evicting recovery data should probably apply to the cached information. On the OPFS parent process side, the oldest unaccessed temporary files could probably be removed as part of the maintenance, with some reasonable eviction rule, or with more effort, the removal could be coupled to the tab session history.

Hm, is this describing the case when the main process suddenly crashes ?
Normally, we should be able to cleanup temporary files even when a content process suddenly crashes, but if the parent process suddenly crashes we need to have some kind of startup cleanup (for example as part of the origin initialization) or async cleanup triggered periodically or after a main process crash.

Flags: needinfo?(jvarga)

It would be good to know how the "It is expected that this atomically updates the contents of the file on disk being written to" would look like in the parent process. This can be tricky for two reasons:

  1. We have to assume that the parent process can crash at any point of the replacement operation, so we need to be able to recover from that.
  2. With dedicated task queues for writable file stream the entire replacement task becomes much more complex, because when the request for explicit close arrives, actor for other writable file stream (but for the same file) can be asynchronously copying content of the main file to a temporary file. The copying must use the old version of the file to preserve correct ordering of requests.

Maybe the initial implementation shouldn't use dedicated task queues for writable file streams to avoid too much complexity in one step.

Maybe bug 1799595 should depend on this and not vice versa because we can't enable shared locks without support for temporary files. We can land support for temporary files while still using exclusive locks for writable file streams.

(In reply to Jan Varga [:janv] from comment #11)

It would be good to know how the "It is expected that this atomically updates the contents of the file on disk being written to" would look like in the parent process. This can be tricky for two reasons:

  1. We have to assume that the parent process can crash at any point of the replacement operation, so we need to be able to recover from that.
  2. With dedicated task queues for writable file stream the entire replacement task becomes much more complex, because when the request for explicit close arrives, actor for other writable file stream (but for the same file) can be asynchronously copying content of the main file to a temporary file. The copying must use the old version of the file to preserve correct ordering of requests.

Maybe the initial implementation shouldn't use dedicated task queues for writable file streams to avoid too much complexity in one step.

We just update the mapping between the entry and the name of the file on disk in the database, and the database offers the atomicity guarantee. Nothing needs to be copied during the replacement operation. The copy takes place when the writable is created and this may fail but does not lead to data corruption.

The argument (from :janv) regarding the message ordering problem is the following: indexedDB does not have bug reports related to this, and since it would work the same way for FileSystemWritableFileStreams, a way handle this issue is probably not needed.

Continuing to copy a file, and sending the copies to the content processes after receiving a close operation which invalidates them is also an optimization and can be done later, although it's not clear if this is a particularly costly optimization.

This generally sounds good. See https://github.com/whatwg/fs/issues/17 about bfcache issues, which aren't yet in the spec, but the plan there should work well. The simplest solution is the ban pages from the bfcache if they have any sort of lock; check if that is currently happening - I can't remember if I implemented that yet or not.

Flags: needinfo?(rjesup)

(In reply to Jan Varga [:janv] from comment #9)

(In reply to Jari Jalkanen from comment #0)

According to fs spec, a FileSystemWritableFileStream has an associated buffer. This buffer can get arbitrarily large, so it is expected that implementations will not keep this in memory, but instead use a temporary file for this. All access to buffer is done in promise returning methods and algorithms, so even though operations on it seem sync, implementations can implement them async.

Any changes made through stream won’t be reflected in the file entry locatable by fileHandle’s locator until the stream has been closed. User agents try to ensure that no partial writes happen, i.e. the file will either contain its old contents or it will contain whatever data was written through stream up until the stream has been closed.

This is typically implemented by writing data to a temporary file, and only replacing the file entry locatable by fileHandle’s locator with the temporary file when the writable filestream is closed.

If keepExistingData is false or not specified, the temporary file starts out empty, otherwise the existing file is first copied to this temporary file.

Creating a FileSystemWritableFileStream takes a shared lock on the file entry locatable with fileHandle’s locator. This prevents the creation of FileSystemSyncAccessHandles for the entry, until the stream is closed.

I would also add that the spec mentions in the closeAlgorithm section "Note: It is expected that this atomically updates the contents of the file on disk being written to." before release the lock.

There's also this comment: "See WICG/file-system-access issue #67 for discussion around and desire for a "inPlace" mode for createWritable (where changes will be written to the actual underlying file as they are written to the writer, for example to support in-place modification of large files or things like databases). This is not currently implemented in Chrome. Implementing this is currently blocked on figuring out how to combine the desire to run malware checks with the desire to let websites make fast in-place modifications to existing large files. In-place writes are available for files in the origin private file system via the FileSystemSyncAccessHandle interface. "

Which is very similar to our current implementation. Maybe we should design our support for temporary files in a way that keeps support for the "old" behavior.

Flags: needinfo?(smaug)
Flags: needinfo?(hsingh)
Flags: needinfo?(bugmail)

(In reply to Jari Jalkanen from comment #13)

(In reply to Jan Varga [:janv] from comment #11)

It would be good to know how the "It is expected that this atomically updates the contents of the file on disk being written to" would look like in the parent process. This can be tricky for two reasons:

  1. We have to assume that the parent process can crash at any point of the replacement operation, so we need to be able to recover from that.
  2. With dedicated task queues for writable file stream the entire replacement task becomes much more complex, because when the request for explicit close arrives, actor for other writable file stream (but for the same file) can be asynchronously copying content of the main file to a temporary file. The copying must use the old version of the file to preserve correct ordering of requests.

Maybe the initial implementation shouldn't use dedicated task queues for writable file streams to avoid too much complexity in one step.

We just update the mapping between the entry and the name of the file on disk in the database, and the database offers the atomicity guarantee. Nothing needs to be copied during the replacement operation. The copy takes place when the writable is created and this may fail but does not lead to data corruption.

Ok, adding support for storing mappings in the database sounds good.

Blocks: 1824993
Blocks: 1823445
Blocks: 1824988
Blocks: 1825018
Depends on: 1818718
Attachment #9330835 - Attachment description: WIP: Bug 1824305 - Implement temporary files for FileSystemWritableFileStream. r=#dom-storage → Bug 1824305 - Implement temporary files for FileSystemWritableFileStream. r=#dom-storage
Attachment #9331849 - Attachment description: WIP: Bug 1824305 - Enable keepExistingData for WritableFileStreams. r=#dom-storage → Bug 1824305 - Enable keepExistingData for WritableFileStreams. r=#dom-storage
Attachment #9331858 - Attachment description: WIP: Bug 1824305 - Have tests for open WritableFileStream bugs. → WIP: Bug 1824305 - Have tests for open WritableFileStream bugs. r=#dom-storage
Attachment #9331858 - Attachment description: WIP: Bug 1824305 - Have tests for open WritableFileStream bugs. r=#dom-storage → Bug 1824305 - Have tests for open WritableFileStream bugs. r=#dom-storage

Depends on D179561

Depends on D179562

Depends on D179568

Attachment #9336793 - Attachment description: Bug 1824305 - Allow GetFile to return names copies of a file. r=#dom-storage → Bug 1824305 - Allow GetFile to return named copies of a file. r=#dom-storage
Pushed by jjalkanen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b31485aba329
Pass abort flag on WritableFileStream close. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/d83117c5ab6b
Stop failing test if previous test has not created a file. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/5958551b3c81
Rename FindDescendants method. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/0e94318af87d
Add not equal operator to FileId type. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/4baca717ca2f
Introduce FileManager method to create a file as a copy. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/2a75757f18b3
Introduce method to get main file EntryId for given FileId. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/d5fbc7854921
Handle removal of temporary files for a file entry. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/2bc4a335174c
Move GetKnownUsage function to DatabaseManager implementation 002. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/def7e855ae2d
Enable updating main file for a given entry. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/710d7efd2912
Return new FileId value from AddFileId. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/671b9dfe5f2b
Handle locking of temporary and main files. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/e4517877cfee
Allow GetFile to return named copies of a file. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/fac936d680ef
Ensure temporary FileId for WritableFileStreams. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/7c47d917d814
Preserve temporary FileId in parent of WritableFileStream. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/47fe1b6b0cf9
Implement temporary files for FileSystemWritableFileStream. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/f05294181879
Enable keepExistingData for WritableFileStreams. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/8102b3d788e7
Pass more wpt tests for WritableFileStreams. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/a750f83a2078
Have tests for open WritableFileStream bugs. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/c272b4e1050a
Align FileSystemDatabaseManager headers with core guideline C128. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/35ad10d171a1
Use consistent ordering for FileSystemDatabaseManager headers. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/896725d40194
Use consistent ordering for FileSystemDatabaseManagerVersion002 implementations. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/acfca9f6e2f6
Use consistent ordering for FileSystemDatabaseManagerVersion001 implementations. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/d3a226834b21
Reorganize WritableFileStream closing and aborting methods. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/858cb1f4d162
Retain exclusive mode for WritableFileStreams for old schema. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/946cb4b832a2
Override GetFile implementation for OPFS version 002. r=dom-storage-reviewers,janv
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch
Blocks: 1841726
Regressions: 1844619
Regressions: 1845304
Regressions: 1858820
Regressions: 1874334
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: