search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.88k stars 29.16k forks source link

UV_USE_IO_URING does not update file handle position #55113

Open ronag opened 5 days ago

ronag commented 5 days ago

I suspect possible regression. Not sure what version but must have been recent:

await stream.promises.pipeline(src, fs.createWriteStream(dstPath))

Will not create a full sized file. I suspect that since createWriteStream doesn't explicitly set position during writes it expects the file handle position to be properly updated, which does not seem to be the case with URING.

santigimeno commented 5 days ago

Yes, ordering in the handling of requests sent to the ring is not guaranteed. So you're right, the offset should be properly set when calling uv_fs_write().

ronag commented 5 days ago

I think we should throw in fs.write if position is not provided while UV_USE_IO_URING is enabled. Do we have any other such instances? net?

ronag commented 5 days ago

@nodejs/tsc I think this is so bad that we should just remove UV_USE_IO_URING until we figure out a way to fix it.

santigimeno commented 5 days ago

This should not be an issue once v1.49.0, which was just released, is used as it disables the SQPOLL ring by default.

mcollina commented 5 days ago

Didn't we disable UV_USE_IO_URING by default everywhere?

ronag commented 5 days ago

Didn't we disable UV_USE_IO_URING by default everywhere?

Yes, referring to a security issue. But some users like us enable it without knowing that here be dragons.

santigimeno commented 5 days ago

@ronag wait, I'm not familiar with the internals of the streams. Can those uv_fs_write() ops happen in parallel or sequentially? If the latter, I think it should work.

ronag commented 5 days ago

They are sequential.

santigimeno commented 5 days ago

Ok, can you share a reproducer?