fix: Use buffered reader in peer to fix ShortBuffer #303
Conversation
This prevents a io.ErrShortBuffer from occurring when the byte slice being read is smaller than the chunks sent from the opposite pipe. This makes sense for unordered connections, where transmission is not guaranteed, but does not make sense for TCP-like connections. We use a bufio.Reader when ordered to ensure data isn't lost.
Codecov Report
@@ Coverage Diff @@
## main #303 +/- ##
==========================================
- Coverage 67.85% 67.75% -0.11%
==========================================
Files 135 135
Lines 7149 7154 +5
Branches 73 73
==========================================
- Hits 4851 4847 -4
- Misses 1809 1815 +6
- Partials 489 492 +3
Continue to review full report at Codecov.
|
| // When ordered, this adds a bufio.Reader. This ensures additional | ||
| // data on TCP-like connections can be read in parts, while still | ||
| // being bufferred. | ||
| if c.opts.Unordered { |
There was a problem hiding this comment.
ooo, wonder if we have the same issue with wsnet 🤔
There was a problem hiding this comment.
I was really confused why wsnet doesn't have this problem, but I think it's because we pipe using net.Dial for everything. So the problem exists, but it's hidden behind the TCP buffering there maybe?
ssh is a specific protocol in the agent, so we're exchanging much smaller payloads here, which made me notice this bug.
There was a problem hiding this comment.
for context, I was getting weird short buffer errors (that I had no idea how to reproduce easily or diagnose) when I tried to remove the envagent proxying, so I wonder whether this would've fixed it
There was a problem hiding this comment.
Oh, this almost certainly would have!
| // When ordered, this adds a bufio.Reader. This ensures additional | ||
| // data on TCP-like connections can be read in parts, while still | ||
| // being bufferred. |
There was a problem hiding this comment.
Thanks for documenting this, very helpful. Wouldn't have been clear to me why we need this otherwise 👍
| // When ordered, this adds a bufio.Reader. This ensures additional | ||
| // data on TCP-like connections can be read in parts, while still | ||
| // being bufferred. | ||
| if c.opts.Unordered { |
There was a problem hiding this comment.
Curious if we use unordered connections via WebRTC in the product today?
This prevents a io.ErrShortBuffer from occurring when the byte
slice being read is smaller than the chunks sent from the opposite
pipe.
This makes sense for unordered connections, where transmission is
not guaranteed, but does not make sense for TCP-like connections.
We use a bufio.Reader when ordered to ensure data isn't lost.