[WasmFS] Allow backends to report errors on setSize#17768
Conversation
Add a test that exercises the `set_size_file` path in the OPFS backend since it should not be possible to get an error on the `set_size_access` codepath. (Although I wouldn't be surprised if it is possible in a way I haven't thought of.) Also, as a drive-by, fix an error code in `doTruncate` to be properly negative.
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
|
|
||
| _wasmfs_opfs_set_size_access__deps: ['$wasmfsOPFSAccessHandles'], | ||
| _wasmfs_opfs_set_size_access: async function(ctx, accessID, size) { | ||
| _wasmfs_opfs_set_size_access: async function(ctx, accessID, size, errPtr) { |
There was a problem hiding this comment.
In a PR I am writing I used a return value for the error, using the standard 0 for success and error otherwise. Is there a reason not to do it that way?
There was a problem hiding this comment.
Since this function is async, it actually returns a promise and you would have to be able to await that promise to get the return value you intended.
There was a problem hiding this comment.
An async function can return a non-promise value too (it's ok to await 42). So I think we could do it that way too?
There was a problem hiding this comment.
Well, maybe it's simpler this way, I don't feel strongly.
There was a problem hiding this comment.
Yes, but only if it returns before it does its first await, since await requires suspending the computation and returning to the event loop.
There was a problem hiding this comment.
Oh right. But main does an await foo(1). I'm pretty sure that foo(1) is returning a promise...
There was a problem hiding this comment.
I'm not sure what it does internally, good question. But we can just write return 42; in an async function, and it just works. So we could do this another way, I think, but as above it's probably less simple (so lgtm as is).
There was a problem hiding this comment.
Actually, even without the internal awaits this returns a promise.
Welcome to Node.js v18.6.0.
Type ".help" for more information.
> async function foo() { return 42; }
undefined
> foo()
Promise {
42,
[Symbol(async_id_symbol)]: 246,
[Symbol(trigger_async_id_symbol)]: 5
}
There was a problem hiding this comment.
Ah, ok, I guess that internally then an async function returning 42 turns that 42 into a resolved Promise of 42...
There was a problem hiding this comment.
And IIUC, awaiting still requires a return to the microtask queue even the value is immediately available, so there's really no way around returning via a pointer here.
| return -EACCES; | ||
| } | ||
| } else { | ||
| if (!child->is<DataFile>()) { |
There was a problem hiding this comment.
Should there be a TODO here for symlinks?
There was a problem hiding this comment.
No, because symlinks are resolved during path parsing. It's not possible for us to observe a symlink here. We already have an assertion for this on line 469.
2 participants
Add a test that exercises the
set_size_filepath in the OPFS backend since itshould not be possible to get an error on the
set_size_accesscodepath. (Although I wouldn't be surprised if it is possible in a way I haven't
thought of.) Also, as a drive-by, fix an error code in
doTruncateto beproperly negative.