fix(posix): ensure nonblocking file descriptor is nonblocking (#10080) #10303
Conversation
There was a problem hiding this comment.
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:
- If stdin is a TTY, we use
ttyname_rto re-open stdin as non-blocking without being observable to the other process - If stdin is a pipe, it should be treated as blocking and handled accordingly
- If stdin is a regular file, we do not poll it since that is not supported anyway
- If stdin is a socket, we use pass
MSG_NOWAITtorecvandsend - If stdin is a pipe on Linux and
pwritev2is 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.
|
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 I spawned the process with a pipe as I needed to replicate the regressive behaviour I was experiencing when using Bun |
|
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.
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. |
| @@ -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; | |||
There was a problem hiding this comment.
| 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
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? |
|
This pull request is stale and may be closed due to inactivity. |
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
EAGAINerror. 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.How did you verify your code works?
I created a new regression test for the specific edge case.