Open mash-graz opened 5 months ago
This is a bug in fs.open()
and not in node:worker_threads
. Most polyfills in fs
are currently wrong as they don't work on actual file descriptors but Deno's own "resources". This is outright wrong and needs to be fixed.
I'm not sure, if I'm able to understand your explanation.
Right now all operations in /ext/node/polyfills/_fs/...
just translate the calling options to finally call their counterparts in ext/fs
. And this functions use the resource table as an additional indirection instead of real file descriptors. The file descriptors are present in this table, but they can't be easily shared between mainThread and worker.
And if I got it right, you argue, that the /ext/node
file operations shouldn't take this indirection over the resource table, but simply use and return plain file descriptor numbers to achieve better compatibility.
The file descriptors are present in this table, but they can't be easily shared between mainThread and worker.
No, they are not. Resource ID is not a File Descriptor. Even though they are both number, the rid
does not correspond to the actual value of FD of the OS.
And if I got it right, you argue, that the /ext/node file operations shouldn't take this indirection over the resource table, but simply use and return plain file descriptor numbers to achieve better compatibility.
We might still use resource table to properly clean up opened files, but we first need to fix the APIs to return proper OS file descriptors instead of resource ids. This is gonna be a big refactor that will take about two weeks and needs to be handled by the core team.
No, they are not. Resource ID is not a File Descriptor. Even though they are both number, the rid does not correspond to the actual value of FD of the OS.
I never thought, that Resource IDs
in deno
are identical to file descriptors.
But we can already get the file descriptors from an entry in the Resource table simply by:
#[op2(fast)]
pub fn op_fs_get_fd(
state: &mut OpState,
#[smi] rid: ResourceId,
) -> Result<i32, AnyError>{
let file = FileResource::get_file(state, rid)?;
let fd = match file.backing_fd() {
Some(fd) => fd,
None => return Err(generic_error("fd not found")),
};
Ok(fd)
}
That's why I'm rather confused about your explanation, that file descriptors aren't accessible via this RIDs.
The question, how these handlers or entire resource entries should be shared/replicated/transferred between threads/workers without fear, looks much more difficile to me.
Nevertheless, I really understand that this topic is indeed a rather complex one and it has to be handled very carefully by those, who are really familiar with its design and security implications.
A bunch of tests were disabled in https://github.com/denoland/deno/pull/25285 for these APIs as they showed incorrect behavior using resource IDs instead of file descriptors.
Version: upstream (2f7b9660)
deno
seems to show a significant different behavior when it comes to accessibility of resources between main instance and workers. Innode
andbun
you can open a file, send the file descriptor resp. RID to the worker and access its content in this other context. Using deno, you will get just anBadResource
Error. The received RID isn't present in the resource table of the client.Here a small demonstration script:
the output using
deno
:on
node
:and
bun
:Although I understand
deno
s radical different answer in this case very well, because it helps to prevent all sort of thread related troubles, it's still a significant different behavior resp. incompatibility.The
deno_core
docu states aboutResources
:I'm not sure if this explanation inevitable applies to
node:worker_threads
, therefore I'm asking here, how we should work around this incompatibility?Otherwise I would have to write rather complex adaptations for an existing software to bypass this differences in behavior (https://github.com/bytecodealliance/jco/issues/400).