Repository navigation
OPFSAdaptiveVFS: release the open lock when an open fails - #369
Conversation
Without readwrite-unsafe access handles, jOpen takes the file's Web Lock and then its access handle. When the handle cannot be acquired - the file held by another context, or by a worker just terminated whose handles the engine has not reclaimed yet - the catch returns SQLITE_CANTOPEN with the lock still held and the request channel open. SQLite does not call xClose after a failed xOpen, so nothing releases them. Every later open of that file then waits for the lock forever, in the same worker and in others, until the worker that failed terminates. The catch now releases the lock, closes the channel and forgets the file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A worker holds an exclusive access handle on the database file, so the open fails - which is expected. The holder then lets go and the same worker opens the database again, which has to succeed. Against the previous behaviour it never completed, waiting for the lock the failed open had kept; the test bounds it at 5 s and reports it as hung. The path is the one taken without readwrite-unsafe access handles, as on Firefox. The test worker removes FileSystemSyncAccessHandle's mode before loading the VFS, so that Chromium takes it too. The holder from vfs_handle_recovery.js is exported for this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Hmm. When I patch my local OPFSAdaptiveVFS:
When I run the demo (http://localhost:8000/demo/?build=jspi&config=OPFSAdaptiveVFS&reset) the console shows a call to And the stack trace from If this is a representative sequence then I think the primary problem is that |
SQLite calls xClose after a failed xOpen, since wa-sqlite's glue sets pMethods whatever xOpen returns; jClose is where the open lock belongs.
|
Thanks for checking. You're right, and that sentence was wrong: I've moved the main fix there, so I merged master into the branch and updated the description. The test fails on master's VFS and passes 3 runs of 3; the whole suite has 6092 passed, 0 failed. |
Is there a reason not to release the access handle lock (if it exists) and close the channel? |
Left to a later request on the channel, the lock outlived the file and the channel stayed open.
|
No reason beyond keeping the change small. Done: The test passes 3 runs of 3; whole suite 6092 passed, 0 failed. |
|
Great, thanks! LGTM. |
…#369) * Release the open lock when an OPFSAdaptiveVFS open fails. Without readwrite-unsafe access handles, jOpen takes the file's Web Lock and then its access handle. When the handle cannot be acquired - the file held by another context, or by a worker just terminated whose handles the engine has not reclaimed yet - the catch returns SQLITE_CANTOPEN with the lock still held and the request channel open. SQLite does not call xClose after a failed xOpen, so nothing releases them. Every later open of that file then waits for the lock forever, in the same worker and in others, until the worker that failed terminates. The catch now releases the lock, closes the channel and forgets the file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add an OPFSAdaptiveVFS test for recovery after a failed open. A worker holds an exclusive access handle on the database file, so the open fails - which is expected. The holder then lets go and the same worker opens the database again, which has to succeed. Against the previous behaviour it never completed, waiting for the lock the failed open had kept; the test bounds it at 5 s and reports it as hung. The path is the one taken without readwrite-unsafe access handles, as on Firefox. The test worker removes FileSystemSyncAccessHandle's mode before loading the VFS, so that Chromium takes it too. The holder from vfs_handle_recovery.js is exported for this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Release the open lock in OPFSAdaptiveVFS's jClose. SQLite calls xClose after a failed xOpen, since wa-sqlite's glue sets pMethods whatever xOpen returns; jClose is where the open lock belongs. * Release the access-handle lock and close the channel in jClose. Left to a later request on the channel, the lock outlived the file and the channel stayed open. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
As mentioned in #365.
What happens
Without
readwrite-unsafeaccess handles, as on Firefox,jOpentakes the file's Web Lock and then its access handle, and keeps the lock until the firstjRead. When the handle cannot be acquired - the file held by another context, or by a worker just terminated whose handles the engine has not reclaimed yet -jOpenreturnsSQLITE_CANTOPENwith the lock still held and the request channel open. SQLite then callsxCloseon the file, butjCloseonly closes the access handle, so nothing releases the lock.Every later open of that file then waits for the lock forever, in the same worker and in others, until the worker that failed terminates.
navigator.locks.query()shows the lock (OPFS:/<file>) held after the failed open.The change
jClosereleases the open lock and the access-handle lock if the file still holds them, and closes the request channel. ThecatchinjOpenalso releases the open lock, closes the channel and removes the file frommapIdToFile, so a failed open leaves nothing forjCloseto clean up. Nothing changes wherereadwrite-unsafehandles exist, since no lock is taken there. On success, the open lock is still released by the first read.Test
test/vfs_open_lock_recovery.jswith a worker of its own, wired intotest/OPFSAdaptiveVFS.test.js. A worker holds an exclusive handle on the database file, so the open fails; the holder lets go, and the same worker must then open the database. The reopen is bounded at 5 s and reported as hung. The test worker removesFileSystemSyncAccessHandle'smodebefore loading the VFS, so Chromium takes the path of engines withoutreadwrite-unsafe. It is skipped where the engine grants a second handle on the same file.On
master:Expected 'hung' to be 'opened'.on asyncify and jspi. With this change the file's 70 tests pass, 3 runs of 3. The same scenario on Firefox itself (outside the suite, which runs Chromium) hangs onmasterand opens with this change.It reuses the holder that
vfs_handle_recovery.jsexports since #367.The whole suite on this branch, with master merged in: 6092 passed, 0 failed.
Checklist
non-exclusive, royalty-free, irrevocable copyright license to reproduce, prepare
derivative works of, publicly display, sublicense, and distribute this
Contribution and such derivative works.
Contribution contains no content requiring a license from any third party.