Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(posix): ensure nonblocking file descriptor is nonblocking (#10080) #10303

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexkornitzer
Copy link

@alexkornitzer alexkornitzer commented Apr 16, 2024

What does this PR do?

The FileReader is responsible for deciding what sort of file descriptor has been passed to it. On MacOS, when stdin is provided to it the FileReader assumes that it is nonblocking even if the descriptor is not, this resulted in hangs as the socket would never raise the EAGAIN error. There are a couple of ways to fix this but the one that seemed to make the most sense was to set the fd to be nonblocking when the FileReader deemed it so.

  • Code changes

How did you verify your code works?

I created a new regression test for the specific edge case.

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this

Unfortunately, we cannot change the stdin file descriptor to be non-blocking or it will break a process which is piped into Bun (like cat foo | bun bar.js) when enough input has been written to cause EAGAIN

What we do instead:

  1. If stdin is a TTY, we use ttyname_r to re-open stdin as non-blocking without being observable to the other process
  2. If stdin is a pipe, it should be treated as blocking and handled accordingly
  3. If stdin is a regular file, we do not poll it since that is not supported anyway
  4. If stdin is a socket, we use pass MSG_NOWAIT to recv and send
  5. If stdin is a pipe on Linux and pwritev2 is supported, we use the flag to only write if it can do so non-blockingly

Furthermore, when spawning processes, we use a socket instead of a blocking pipe which avoids this case

That leaves 2 as the likely scenario where this is happening. So the next step here is to figure out why nonblocking is being set to true despite this happening.

@alexkornitzer
Copy link
Author

Hey @Jarred-Sumner,

Sorry to force your hand to reply by creating a PR, was hoping for this feedback in the original issue but am aware that you have a flood of them with no labels for prioritisation and/or triaged status.

If 2 is the intended behaviour then it will be an incorrect assumption in that function so am happy to dig that out.

I spawned the process with a pipe as I needed to replicate the regressive behaviour I was experiencing when using Bun 1.0.31+ via Erlang.

@alexkornitzer
Copy link
Author

Did some lazy debugging and the issue here is the assumption that a pollable non atty is nonblocking, but the point of truth here is on the descriptor via the O_NONBLOCK flag.

[SYS] is_nonblocking_tty: false, pollable: true, file.is_atty: false, O_NONBLOCK: false

So if mutating the descriptor is unsafe the only other solution I can think of here with my very basic understanding is to just check the flag as gospel. Which was going to be my other approach to fix this but I had assumed the descriptor was being forced to nonblocking for a reason hence my original approach.

Copy link
Contributor

github-actions bot commented May 1, 2024

@gvilums, your commit has failing tests :(

💪 2 failing tests Darwin AARCH64

💻 3 failing tests Darwin x64 baseline

💻 2 failing tests Darwin x64

🪟💻 10 failing tests Windows x64 baseline

🪟💻 8 failing tests Windows x64

View logs

@@ -514,6 +514,10 @@ pub fn ensureNonBlocking(fd: anytype) void {
_ = std.os.fcntl(fd, std.os.F.SETFL, current | std.os.O.NONBLOCK) catch 0;
}

pub fn isNonBlocking(fd: anytype) bool {
return (std.os.fcntl(fd, std.os.F.GETFL, 0) catch 0 & std.os.O.NONBLOCK) == std.os.O.NONBLOCK;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return (std.os.fcntl(fd, std.os.F.GETFL, 0) catch 0 & std.os.O.NONBLOCK) == std.os.O.NONBLOCK;
if (Environment.isWindows) {
@compileError("isNonBlocking is not supported on windows");
}
return (std.os.fcntl(fd, std.os.F.GETFL, 0) catch 0 & std.os.O.NONBLOCK) == std.os.O.NONBLOCK;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking at this, do you need any action from me? Would you like me to rebase against master and apply the above suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that would be great 👍

Copy link
Author

@alexkornitzer alexkornitzer May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I tried to align the formatting to that of other @compileError functions within the file. I also squashed the formatting commit into the original commit to keep the git history clean.

The FileReader is responsible for deciding what sort of file descriptor
has been passed to it. On MacOS, when stdin is provided to it the
FileReader assumes that it is nonblocking even if the descriptor is not
as it passes the pollable and not a tty check, this then results in
hangs as the reader expects `EAGAIN`.

To fix this we now check the file descriptor to see if it is nonblocking
rather than using assumptions.
Ensure that this function is not called on Windows as it is not
supported being a POSIX only function.
@alexkornitzer
Copy link
Author

Hey team, I understand that this bug may not be very high priority due to the number of people that it is or maybe more accurately is not affecting, but would it be possible for a decision to be made regarding this PR?

Copy link
Contributor

github-actions bot commented Aug 8, 2024

This pull request is stale and may be closed due to inactivity.

@github-actions github-actions bot added the stale label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants