Skip to content

UV_USE_IO_URING does not update file handle position #55113

Open

Description

@ronag

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.

Activity

  1. added
    fsIssues and PRs related to file-system APIs and the fs module.
    libuvIssues and PRs related to the libuv dependency or the uv binding.
    on Sep 25, 2024
  2. santigimeno commented on Sep 25, 2024

    @santigimeno
    Member

    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().

  3. ronag commented on Sep 25, 2024

    @ronag
    MemberAuthor

    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?

  4. ronag commented on Sep 25, 2024

    @ronag
    MemberAuthor

    @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.

  5. santigimeno commented on Sep 25, 2024

    @santigimeno
    Member

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

  6. mcollina commented on Sep 25, 2024

    @mcollina
    SponsorMember

    Didn't we disable UV_USE_IO_URING by default everywhere?

  7. ronag commented on Sep 25, 2024

    @ronag
    MemberAuthor

    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.

  8. santigimeno commented on Sep 25, 2024

    @santigimeno
    Member

    @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.

  9. ronag commented on Sep 25, 2024

    @ronag
    MemberAuthor

    They are sequential.

  10. santigimeno commented on Sep 25, 2024

    @santigimeno
    Member

    Ok, can you share a reproducer?

  11. added
    linuxIssues and PRs related to the Linux platform.
    on Oct 31, 2024
  12. juanarbol commented on Nov 10, 2024

    @juanarbol
    Member

    ping @ronag, I would like to help on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.fsIssues and PRs related to file-system APIs and the fs module.libuvIssues and PRs related to the libuv dependency or the uv binding.linuxIssues and PRs related to the Linux platform.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions