Additional policy for limiting RTT PINGs in SocketsHttpHandler #70995
Conversation
ghost
added
the
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis PR implements an additional policy for limiting RTT/BDP PINGs to comply with the PING accounting mechanism of a specific server in google's backends. Long story short, we need to make sure we do not send more than 4 PINGs without also sending DATA, HEADERS or WINDOW_UPDATE. Thanks to the update mentioned in grpc/grpc-dotnet#1765, I believe #62216 would only occur in extreme corner cases which I was able to emulate in a functional test, but cannot reproduce in real life. I think the fix is still valuable to make 100% sure we are compliant with google's backends and avoid rare sporadic bugs in the future, but I don't think we need to backport it to 6.0. Fixes #62216. Contributes to #69870 by making
|
| if (NetEventSource.Log.IsEnabled()) s.thisRef.Trace(s.http2Stream.StreamId, $"Wrote HEADERS frame. Length={current.Length}, flags={flags}"); | ||
|
|
||
| // RTT PING bookeeping | ||
| s.thisRef._rttEstimator.OnDataOrHeadersOrWindowUpdateSent(); |
There was a problem hiding this comment.
OnDataOrHeadersOrWindowUpdateSent is implemented with an interlocked operation. I assume the perf impact is marginal compared to the buffer copying code above, but I may be wrong.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| BinaryPrimitives.WriteInt64BigEndian(span.Slice(FrameHeader.Size), state.pingContent); | ||
|
|
||
| // RTT PING bookeeping | ||
| state.thisRef._rttEstimator.OnPingSent(); |
There was a problem hiding this comment.
If I understand this correctly, this will count ACKs as well. Is that wanted?
There was a problem hiding this comment.
Yes. My assumption was that all pings are being accounted on the server side, regardless of the ACK flag. Even if it isn't the case, this stricter policy shouldn't cause a problem here, unless the server is also BDP/RTT measuring us intensively with PINGs and AFAIK servers don't do it.
@jtattermusch do you have any insights which are OK to share publicly?
| internal void OnDataOrHeadersReceived(Http2Connection connection) | ||
| { | ||
| if (_state != State.Waiting) return; | ||
| if (_sendPolicyCounter >= MaxPingsWithoutSending) return; |
There was a problem hiding this comment.
Is this supposed to be a hard limit?
Am I reading the code correctly? This will check the value before putting the PING into the write queue, but there might be some unprocessed pending PING frames which would increment the counter, thus leading to more than MaxPingsWithoutSending PINGs sent. Or I'm missing something here?
There was a problem hiding this comment.
You are right, I will try to implement a second check in SendPingAsync to deal with this.
There was a problem hiding this comment.
Thinking about this, it looks like there is no easy way to get rid of these races without introducing a lot of additional complexity around this code, and I'm not sure if it's worth it.
I'm tempted to close this PR and the issue #62216, since I'm not sure we are trying to solve a problem that actually exist in practice after google's backend tuning. We can come back and work on a more sophisticated solution if we get new user reports.
|
Triage: We decided to not pursue the PR further, given the race conditions discovered above. |
3 participants
This PR implements an additional policy for limiting RTT/BDP PINGs to comply with the PING accounting mechanism of a specific server in google's backends.
Long story short, we need to make sure we do not send more than 4 PINGs without also sending DATA, HEADERS or WINDOW_UPDATE.
Thanks to the update mentioned in grpc/grpc-dotnet#1765, I believe #62216 would only occur in extreme corner cases which I was able to emulate in a functional test, but cannot reproduce in real life. I think the fix is still valuable to make 100% sure we are compliant with google's backends and avoid rare sporadic bugs in the future, but I don't think we need to backport it to 6.0.
Fixes #62216.
Contributes to #69870 by making
SocketsHttpHandler_Http2FlowControl_Testtests conditional onSocketsHttpHandler.IsSupported.