[WasmFS] Track open files in the Node backend #16432
Conversation
Add callbacks to the DataFile API for notifying backends when files are opened or closed, and call them from the OpenFileEntry constructor and destructor. In the Node backend, open and close files on the underlying file system in these callbacks and track the underlying file descriptor for opened files. This is sufficient to allow the Node backend to correctly read and write opened files that have been unlinked. Open unlinked directories do not yet work correctly on the Node backend because WasmFS zeroes the parent pointer on a different File object than the one held in the open file table. Fixing that is left to a follow-on PR.
| } catch (e) { | ||
| if (!e.code) throw e; | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Put the return and wasmfsNodeFixStat call after the try/catch?
src/library_wasmfs_node.js
Outdated
| $wasmfsNodeFstat: function(fd) { | ||
| let stat; | ||
| try { | ||
| return wasmfsNodeFixStat(fs.fstatSync(fd)); |
| @@ -18,6 +18,8 @@ namespace wasmfs { | |||
| class MemoryFile : public DataFile { | |||
| std::vector<uint8_t> buffer; | |||
|
|
|||
| void open() override {} | |||
| void close() override {} | |||
There was a problem hiding this comment.
Maybe just male these default to empty function rather than pure virtual? (seems like overriding them is optional).
There was a problem hiding this comment.
Actually, I expect that all nontrivial backends will need to override these to get the correct behavior for accessing files after they have been unlinked, so I think it would be better to keep these non-optional.
tests/test_other.py
Outdated
| @@ -130,14 +130,33 @@ def metafunc(self, wasmfs): | |||
| return metafunc | |||
|
|
|||
|
|
|||
| WASMFS_BACKEND_MAP = {'': 'WASMFS_MEMORY_BACKEND', | |||
| 'node': 'WASMFS_NODE_BACKEND'} | |||
There was a problem hiding this comment.
I feel like maybe exacting this map makes the two metafunc._parameterize below hard to read.. maybe just duplicate it instead? Feel free to disagree ..
There was a problem hiding this comment.
Yeah, maybe better to be DAMP than DRY here.
|
This test seems to be failing on WIndows: https://ci.chromium.org/ui/p/emscripten-releases/builders/try/win/b8820197662569474913/overview |
|
Ah bummer. I'll just disable it on Windows for now. |
| // The state of a file on the underlying Node file system. | ||
| class NodeState { | ||
| // Map all separate WasmFS opens of a file to a single underlying fd. | ||
| size_t openCount = 0; |
There was a problem hiding this comment.
Should this be atomic? Or does the node backend have some kind of assurance on running in a single thread like the old FS did?
There was a problem hiding this comment.
Core WasmFS only ever calls backend methods while the corresponding file lock is held, so backends generally don't need to do their own additional synchronization unless they share state between files.
Add callbacks to the DataFile API for notifying backends when files are opened
or closed, and call them from the OpenFileEntry constructor and destructor. In
the Node backend, open and close files on the underlying file system in these
callbacks and track the underlying file descriptor for opened files. This is
sufficient to allow the Node backend to correctly read and write opened files
that have been unlinked.
Open unlinked directories do not yet work correctly on the Node backend because
WasmFS zeroes the parent pointer on a different File object than the one held in
the open file table. Fixing that is left to a follow-on PR.