Read stream from BufferedReader

[antontornqvist]

Fixes #3332

[antontornqvist]

@kiview Bump, could you review this PR?

[Park-Jiyeonn]

Hi @kiview, I also have this problem, could you please take a look for this PR when you get a chance? Thx!

[kurtqq]

@akerouanton any chance to review and release this fix?

[fruch]

@antontornqvist thanks for tracking this down — the diagnosis here is right, and it cost me a while to arrive at the same place independently.

I've opened #3438, which keeps the shape of this PR and adds three things I ran into while testing it:

• read1(n) instead of read(n). BufferedReader.read(n) blocks until it has n bytes or hits EOF, so a stream that stays open delivers nothing until 4096 bytes pile up. Against a server that writes one frame and keeps the connection open: read(4096) → nothing after 3s, read1(4096) → the frame immediately. This affects attach(stream=True) and exec_run(stream=True, tty=True). The existing integration tests miss it because cancelling the stream shuts the socket down and the buffered bytes are flushed at EOF.
• The socket timeout. With the poll skipped, the blocking read is the socket's own, so timeout now bounds it — a quiet exec longer than timeout seconds raises TimeoutError where it used to wait indefinitely in poll(). Read upgraded streams through the buffered reader #3438 calls _disable_socket_timeout() in _read_from_socket(), which is the only streaming helper that didn't already.
• npipe PIPE_ENDED. Once read() sees the reader rather than the NpipeSocket, the "pipe ended means EOF" handling stops matching.

Plus a TCPSocketStreamUpgradeTest that reproduces the issue deterministically. The existing TCPSocketStreamTest sleeps 0.2s between the headers and the payload — exactly the case that already works — so the new tests write them with a single wfile.write() instead. Five of the six fail on main; two fail with this PR applied (test_stream_tty and test_stream_quiet, the two points above).

Happy either way: land #3438, or I'll fold those changes into this PR as a review here if you'd prefer to keep authorship where the analysis started. Whichever gets a maintainer to look — this has been open and unreviewed since May, and #2042 since 2018.

[antontornqvist]

@antontornqvist thanks for tracking this down — the diagnosis here is right, and it cost me a while to arrive at the same place independently.

I've opened #3438, which keeps the shape of this PR and adds three things I ran into while testing it:

• read1(n) instead of read(n). BufferedReader.read(n) blocks until it has n bytes or hits EOF, so a stream that stays open delivers nothing until 4096 bytes pile up. Against a server that writes one frame and keeps the connection open: read(4096) → nothing after 3s, read1(4096) → the frame immediately. This affects attach(stream=True) and exec_run(stream=True, tty=True). The existing integration tests miss it because cancelling the stream shuts the socket down and the buffered bytes are flushed at EOF.
• The socket timeout. With the poll skipped, the blocking read is the socket's own, so timeout now bounds it — a quiet exec longer than timeout seconds raises TimeoutError where it used to wait indefinitely in poll(). Read upgraded streams through the buffered reader #3438 calls _disable_socket_timeout() in _read_from_socket(), which is the only streaming helper that didn't already.
• npipe PIPE_ENDED. Once read() sees the reader rather than the NpipeSocket, the "pipe ended means EOF" handling stops matching.

Plus a TCPSocketStreamUpgradeTest that reproduces the issue deterministically. The existing TCPSocketStreamTest sleeps 0.2s between the headers and the payload — exactly the case that already works — so the new tests write them with a single wfile.write() instead. Five of the six fail on main; two fail with this PR applied (test_stream_tty and test_stream_quiet, the two points above).

Happy either way: land #3438, or I'll fold those changes into this PR as a review here if you'd prefer to keep authorship where the analysis started. Whichever gets a maintainer to look — this has been open and unreviewed since May, and #2042 since 2018.

Nice! Either way works for me too, I would just be happy to get a fix for this issue merged.
I don't have high hopes, as I have not heard back from the maintainer on this issue since I raised this PR.