P2PSocketUdp: Retry Send() after ERR_NO_BUFFER_SPACE [chromium/src : main]
Steve Becker (Gerrit)
Steve Becker voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
Please let me know if you have any questions about this change.
Related details
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harald Alvestrand (Gerrit)
Harald Alvestrand added 4 comments![]( "Open in Gerrit")
This CL introduces a new class of packet delay, which may cause packets to be delivered up to 4 seconds late. Do you have appropriate metrics in place to figure out how this will affect delay-based congestion controllers?
(The delay may be exactly the right signal to feed to them, since "no buffer space" is a kind of network congestion .... but it could also be the wrong signal....)
int result_to_record = result;"result" is either a count or a negative error code. You can get this in one line by saying result_to_record = std::min(result, net::OK).
base::UmaHistogramSparse("WebRTC.P2P.UDP.SendRetryResult",Does recording negative values actually work?
Since the tests pass, I assume it does - is this unique to sparse histograms?
histograms.ExpectUniqueSample("WebRTC.P2P.UDP.SendRetryResult", net::OK, 1);Is there a histogram covering Send results? If there is one, should it have recorded one net::OK and one net::ERR_NO_BUFFER_SPACE?
If there isn't one - should there be one?
Related details
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted and added 5 comments![]( "Open in Gerrit")
Votes added by Steve Becker
| Commit-Queue | +1 |
This CL introduces a new class of packet delay, which may cause packets to be delivered up to 4 seconds late. Do you have appropriate metrics in place to figure out how this will affect delay-based congestion controllers?
(The delay may be exactly the right signal to feed to them, since "no buffer space" is a kind of network congestion .... but it could also be the wrong signal....)
I added another histogram to measure the total delay of the retry attempts. Based on discussions with the Windows Networking team, I expect 4 seconds to be much more than required, but it will be best to verify.
We have customers able to consistently repro in their environments. We're going to work with them to evaluate the fix in Canary and Beta.
Thanks again for the code reviews. I really appreciate it!
"result" is either a count or a negative error code. You can get this in one line by saying result_to_record = std::min(result, net::OK).
Done
base::UmaHistogramSparse("WebRTC.P2P.UDP.SendRetryResult",Does recording negative values actually work?
Since the tests pass, I assume it does - is this unique to sparse histograms?
[According to the function comment](https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_functions.h;drc=f929a0c3e751200212538123bfb58c0b491c3f62;l=221), sparse histograms support negative values.
```
// For recording sparse histograms.
// The |sample| can be a negative or non-negative number.
```
[CombinedHttpResponseAndNetErrorCode defines the expected values in enums.xml](https://source.chromium.org/chromium/chromium/src/+/main:tools/metrics/histograms/enums.xml;drc=67ccdac9245f9b639376296466f031c86e5c6ef1;l=1883).
Here's another part of Chromium recording net error codes using sparse histograms.
histograms.ExpectUniqueSample("WebRTC.P2P.UDP.SendRetryResult", net::OK, 1);Is there a histogram covering Send results? If there is one, should it have recorded one net::OK and one net::ERR_NO_BUFFER_SPACE?
If there isn't one - should there be one?
Good idea. I added a new histogram: `WebRTC.P2P.UDP.SendResult`.
Related details
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Harald Alvestrand
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Harald Alvestrand (Gerrit)
Harald Alvestrand added 1 comment![]( "Open in Gerrit")
Approval for p2p. Adding Johannes to approve for the histograms.
Related details
- Johannes Kron
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron added 1 comment![]( "Open in Gerrit")
base::UmaHistogramSparse("WebRTC.P2P.UDP.SendResult", result_to_record);How often is this logged? It may be worth using the corresponding histogram macros, if it's logged at a high frequency.
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_macros.h;l=407
Related details
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steve Becker
| Commit-Queue | +1 |
1 comment
base::UmaHistogramSparse("WebRTC.P2P.UDP.SendResult", result_to_record);How often is this logged? It may be worth using the corresponding histogram macros, if it's logged at a high frequency.
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_macros.h;l=407
Thanks for the tips. I updated to use the macro. There's [another UMA in `P2PSocketUdp` that uses the macro](https://source.chromium.org/chromium/chromium/src/+/main:services/network/p2p/socket_udp.cc;drc=329f8536e0668b0189a63c2465fe5a21c1165a91;l=348) for received packets, which suggests we should probably use the macro for recording sent packets.
Related details
- Johannes Kron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron voted Code-Review+1![]( "Open in Gerrit")
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
Adding Adam Rice to review //net/base/net_errors_win.cc. Please take a look when you have a moment
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
I think this CL will also have the effect that QUIC will also start retrying sends on WSAENOBUFS errors on Windows. This is probably a good thing, but it is worth explicitly mentioning in the CL description, particularly as that part of the change is not behind a feature flag.
base::RunLoop().RunUntilIdle();It's best not to use `RunUntilIdle()` as it leads to flaky tests. Maybe do
```
base::test::RunUntil([&] {
return histograms.GetBucketCount("WebRTC.P2P.UDP.SendResult", net::OK) == 1;
});
```
?
Related details
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 1 comment![]( "Open in Gerrit")
Related details
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted and added 3 comments![]( "Open in Gerrit")
Votes added by Steve Becker
| Commit-Queue | +2 |
3 comments
I think this CL will also have the effect that QUIC will also start retrying sends on WSAENOBUFS errors on Windows. This is probably a good thing, but it is worth explicitly mentioning in the CL description, particularly as that part of the change is not behind a feature flag.
Great point. I updated the commit message.
It's best not to use `RunUntilIdle()` as it leads to flaky tests. Maybe do
```
base::test::RunUntil([&] {
return histograms.GetBucketCount("WebRTC.P2P.UDP.SendResult", net::OK) == 1;
});
```
?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
8 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: services/network/p2p/socket_udp_unittest.cc
Insertions: 4, Deletions: 10.
The diff is too large to show. Please review the diff.
```
Change information
P2PSocketUdp: Retry Send() after ERR_NO_BUFFER_SPACE
Brings QUIC's `ERR_NO_BUFFER_SPACE` retry to `P2PSocketUdp`. See
QuicChromiumPacketWriter::MaybeRetryAfterWriteError() for details:
https://source.chromium.org/chromium/chromium/src/+/main:net/quic/quic_chromium_packet_writer.cc;l=272
In the bug, customers provided logs that show `P2PSocketUdp` failing
with `WSAENOBUFS` on Windows. This change maps `WSAENOBUFS` to
`ERR_NO_BUFFER_SPACE` to match the equivalent POSIX error `ENOBUFS`.
This new error mapping enables the QUIC retry on Windows.
The rest of the change updates `P2PSocketUdp::DoSend()` to retry after
the socket returns `ERR_NO_BUFFER_SPACE`. The retry is on a timer that
starts at 1 millisecond. The timer interval doubles for each retry up to
12 attempts for a total of 4095 milliseconds. The change treats retries
like an async send operation by setting `send_pending_` to true while
waiting for the retry timer.
For diagnostics, this change adds a new log statement and histogram to
determine the effectiveness of the retry.
- M net/base/net_errors_win.cc
- M services/network/p2p/socket_udp.cc
- M services/network/p2p/socket_udp.h
- M services/network/p2p/socket_udp_unittest.cc
- M tools/metrics/histograms/metadata/web_rtc/histograms.xml
Code-Review: +1 by Johannes Kron, +1 by Adam Rice
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |