[Merged by Bors] - p2p: server: fix incorrect bufio usage #6470
Conversation
It is not correct to use `bufio.Buffer` to read the initial request and then passing the underlying stream to the `StreamRequest` callback, as `bufio.Buffer` may happen to read more data than necessary, making it unavailable for the `StreamRequest` callback. This behavior has been obvserved when using QUIC, which tends to coalesce multiple writes more often.
ivan4th
requested review from
dshulyak,
fasmat,
poszu,
acud and
jellonek
as code owners
| func (dadj *deadlineAdjuster) ReadByte() (byte, error) { | ||
| var b [1]byte | ||
| _, err := io.ReadFull(dadj, b[:]) | ||
| return b[0], err | ||
| } |
There was a problem hiding this comment.
Why is io.ReadFull necessary? Doesn't Read guarantee that at least 1 byte is read if err == nil?
| func (dadj *deadlineAdjuster) ReadByte() (byte, error) { | |
| var b [1]byte | |
| _, err := io.ReadFull(dadj, b[:]) | |
| return b[0], err | |
| } | |
| func (dadj *deadlineAdjuster) ReadByte() (byte, error) { | |
| var b [1]byte | |
| _, err := dadj.Read(b[:]) | |
| return b[0], err | |
| } |
There was a problem hiding this comment.
According to io.Reader docs:
Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0. Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.
Thus, while it's not good for implementations to return zero byte count with a nil error with len(p) != 0, this possibility cannot be ruled out completely, and the callers are told explicitly to be able to handle such situations; which io.ReadFull does.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #6470 +/- ##
=========================================
- Coverage 79.9% 79.9% -0.1%
=========================================
Files 353 353
Lines 46432 46435 +3
=========================================
- Hits 37122 37113 -9
- Misses 7210 7219 +9
- Partials 2100 2103 +3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
bors merge |
## Motivation It is not correct to use `bufio.Buffer` to read the initial request and then passing the underlying stream to the `StreamRequest` callback, as `bufio.Buffer` may happen to read more data than necessary, making it unavailable for the `StreamRequest` callback. This behavior has been obvserved when using QUIC, which tends to coalesce multiple writes more often.
|
Pull request successfully merged into develop. Build succeeded: |
spacemesh-bors
bot
changed the title
Motivation
It is not correct to use
bufio.Bufferto read the initial requestand then passing the underlying stream to the
StreamRequestcallback, as
bufio.Buffermay happen to read more data thannecessary, making it unavailable for the
StreamRequestcallback.This behavior has been obvserved when using QUIC, which tends to
coalesce multiple writes more often.
Description
This removes unneeded
bufio.Bufferusage, which should not impactperformance as the buffer was not being used for the actual request
handling, anyway.
Test Plan
Verified that this fixes QUIC write coalescing issue.