-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
stop generating new packets when the send queue is full #2971
Conversation
session.go
Outdated
// nothing to see here. | ||
// We do all the interesting stuff after the switch statement, so | ||
// nothing to see here. | ||
case <-sendQueueAvailable: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to drain sendQueueAvailable
if we end up taking another path in this switch, and then enter the if checks below? Otherwise it could be that we incorrectly assume there's space available even if there isn't, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The select
can unblock for multiple reasons (sending scheduled, receiving a packet, etc.), so we always have to check if there's room in the send queue before sending a packet: https://github.com/lucas-clemente/quic-go/blob/b4ffb97831546fc8336ed5f0c37ed6b6c8c40246/session.go#L621-L629
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I misunderstood your comment. Draining the channel makes sense if WouldBlock()
returns true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that would be racy (the send queue could have unblocked between the calls to WouldBlock
and Available
.
type sendQueue struct { | ||
queue chan *packetBuffer | ||
closeCalled chan struct{} // runStopped when Close() is called | ||
runStopped chan struct{} // runStopped when the run loop returns | ||
available chan struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit difficult to follow, and tightly coupled to run loop internals (e.g. if you use it wrong it's inherently racy, see below).
I wonder whether there's a simpler way of achieving similar behavior? Not sure if it's simpler, but possibly a nextPacket member in the session that we try to send in the session's switch statement?
(If we decide to stick with this, we should probably document the reasoning behind this API.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand why it's inherently race. It's true that the sendQueue
can only be used from a single Go routine (which we guarantee by just running it from the session), but as long as you adhere to this, there shouldn't be any race here.
de0da41
to
3b37e44
Compare
Codecov Report
@@ Coverage Diff @@
## master #2971 +/- ##
==========================================
+ Coverage 86.06% 86.08% +0.02%
==========================================
Files 135 135
Lines 9415 9435 +20
==========================================
+ Hits 8103 8122 +19
- Misses 953 954 +1
Partials 359 359
Continue to review full report at Codecov.
|
a90c6e5
to
dd13de1
Compare
dd13de1
to
72f2092
Compare
72f2092
to
b81a6f8
Compare
Fixes #2941. Depends on #2980.
We shouldn't block the
run
loop in the session ifWriteTo
is too slow to send out packets. If we do that, we won't be able to receive any incoming packets, and thereceivedPackets
channel might overflow, leading to avoidable packet loss.