Add DelayedDatagramSocket: UDP socket with simulated network delay [chromium/src : main]
Patrick Meenan (Gerrit)
Patrick Meenan added 11 comments![]( "Open in Gerrit")
Sorry about the delay. Some issues but nothing huge.
void RegisterQuicConnectionClosePayload(base::span<uint8_t> payload) override;nit: The header should include `"base/containers/span.h"` directly since `base::span` is used in public method signatures (`delayed_datagram_socket.h:182`) and across .cc (`lines 231,347,439,710`).
// ERR_IO_PENDING. Internally the packet is copied into a send queue and anit: Header comments state `Write()` never returns `ERR_IO_PENDING`, while both the unshaped passthrough path (`delayed_datagram_socket.cc:397`) and the direct overflow path (`:428`) delegate directly to `wrapped_socket_->Write()`, which can return `ERR_IO_PENDING`. Consider clarifying in the comment that `Write()` completes synchronously in shaping mode, but forwards `ERR_IO_PENDING` when unshaped (`!rtt.is_positive() && !upload_throttle_`).
inner_read_closed_ = true;Setting `inner_read_closed_ = true` on any `result < 0` permanently stops `EnsureInnerReadInFlight()` after recoverable datagram read errors like `ERR_MSG_TOO_BIG`. Future `Reads` can keep asynchronously surfacing the same latched error instead of restarting the read pump, even though QUIC’s single-read path ignores `ERR_MSG_TOO_BIG` and continues. Can we avoid permanently latching `inner_read_closed_ = true` on recoverable datagram errors (`ERR_MSG_TOO_BIG`), and add a unit test verifying `Read()` error propagation across two hops after an inner read error occurs?
weak_factory_.GetWeakPtr(), std::move(data), packet_tos));When `download_throttle_` is set and a valid zero-length UDP datagram arrives (`result == 0`), passing result to `download_throttle_->RequestBytes(0, ...)` triggers an immediate runtime `CHECK_GT(bytes, 0)` crash in `bandwidth_throttle.cc:107`. Can `ProcessInnerReadResult()` special-case zero-byte packets (`if (result == 0)`) and enqueue them with the normal propagation delay (`OnDownloadThrottleGranted(std::move(data), packet_tos)`) without calling `RequestBytes()`?
int n = std::min(dest_len, static_cast<int>(front.data.size()));When a queued datagram (`front.data.size()`) is larger than the caller's buffer (`dest_len`), `CopyFrontPacketInto()` truncates the datagram to `dest_len`, copies only the truncated bytes, pops `receive_queue_`, and returns `dest_len` as a successful read. POSIX returns `ERR_MSG_TOO_BIG` on truncation (`udp_socket_posix.cc:1141`), and Windows maps `WSAEMSGSIZE` to `ERR_MSG_TOO_BIG`; either way, callers should not receive a successful truncated datagram. If `static_cast<int>(front.data.size()) > dest_len`, can `CopyFrontPacketInto()` return `ERR_MSG_TOO_BIG` and consume/drop the queued datagram (`receive_queue_.pop_front();`) in the same way UDP consumes an oversized datagram from the kernel receive buffer?
NOTREACHED();optional before production wiring: While `UDPSocketWin::ReadMultiple` also executes `NOTREACHED()`, `QuicProxyDatagramClientSocket::ReadMultiple` migrated to `ReadMultipleEmulator` (`quic_proxy_datagram_client_socket.cc:304`) specifically to avoid process crashes when QUIC's `QuicUseReadMultiple` feature (`kQuicUseReadMultiple`) is active (`crbug.com/515333601`). Before wiring this wrapper into production QUIC client flows, can we delegate to `ReadMultipleEmulator` or implement batching through the delay queue?
traffic_annotation);When `send_queue_` saturates (`size() >= kMaxQueuedSendPackets`) and `!inner_write_pending_`, calling `wrapped_socket_->Write()` directly bypasses `send_queue_`. If this direct `wrapped_socket_->Write()` returns `ERR_IO_PENDING`, `Write()` returns `ERR_IO_PENDING` without setting `inner_write_pending_ = true`. When `send_timer_` later fires (`DrainOneWireSend()`), `!inner_write_pending_` passes and `DrainOneWireSend()` issues a second `wrapped_socket_->Write(...)` while the direct write is still pending, causing `UDPSocketPosix` / `UDPSocketWin` to crash on `CHECK(write_callback_.is_null())` (`udp_socket_posix.cc:594`, `udp_socket_win.cc:528`). Can we either set `inner_write_pending_ = true` when line 428 pends asynchronously (and wrap `callback` to clear it and resume draining without dropping caller completion), or consistently drop overflowing packets by returning `buffer_len` when `send_queue_` is full?
return buffer_len;When `Write()` is called in shaping mode (`config_.rtt.is_positive() || upload_throttle_`) before a successful `Connect*()` or when disconnected, `Write()` enqueues `PendingSend` and returns `buffer_len` synchronously without checking connection state. When `DrainOneWireSend()` later writes the packet to `wrapped_socket_`, `wrapped_socket_->Write()` returns `ERR_SOCKET_NOT_CONNECTED`. Because `OnInnerWireSendComplete()` (`:575`) silently drops post-sync errors, the disconnection failure is lost. Can we track connected state in `DelayedDatagramSocket` across successful `Connect*()` / async connect completion and `Close()`, then reject shaped writes with `ERR_SOCKET_NOT_CONNECTED` when not connected?
if (send.dscp != DSCP_NO_CHANGE || send.ecn != ECN_NO_CHANGE) {optional: `PendingSend` snapshots the raw `DSCP_NO_CHANGE` / `ECN_NO_CHANGE` sentinels, then `DrainOneWireSend()` resolves them later against whatever TOS state the inner socket happens to have after earlier queued sends. That can differ from the app-effective TOS at `Write()` time, especially for partial sentinels like `SetTos(DSCP_NO_CHANGE, explicit_ecn)`. Can `DelayedDatagramSocket` maintain an effective DSCP/ECN cache (`effective_dscp_` / `effective_ecn_`) and resolve `NO_CHANGE` sentinels against the cached effective state when constructing `PendingSend` in `Write()`, so `DrainOneWireSend()` applies the exact intended TOS per packet?
TRAFFIC_ANNOTATION_FOR_TESTS);nit: In `OverflowRestoresPerPacketStateWhenInnerIdle` (`line 1374`), the return value of `socket->Write(...)` is discarded on the overflow path. Checking `EXPECT_EQ(socket->Write(...), 8);` ensures overflow return code assumptions are explicitly verified.
Related details
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 10 comments![]( "Open in Gerrit")
void RegisterQuicConnectionClosePayload(base::span<uint8_t> payload) override;nit: The header should include `"base/containers/span.h"` directly since `base::span` is used in public method signatures (`delayed_datagram_socket.h:182`) and across .cc (`lines 231,347,439,710`).
Done
// ERR_IO_PENDING. Internally the packet is copied into a send queue and anit: Header comments state `Write()` never returns `ERR_IO_PENDING`, while both the unshaped passthrough path (`delayed_datagram_socket.cc:397`) and the direct overflow path (`:428`) delegate directly to `wrapped_socket_->Write()`, which can return `ERR_IO_PENDING`. Consider clarifying in the comment that `Write()` completes synchronously in shaping mode, but forwards `ERR_IO_PENDING` when unshaped (`!rtt.is_positive() && !upload_throttle_`).
Done
Setting `inner_read_closed_ = true` on any `result < 0` permanently stops `EnsureInnerReadInFlight()` after recoverable datagram read errors like `ERR_MSG_TOO_BIG`. Future `Reads` can keep asynchronously surfacing the same latched error instead of restarting the read pump, even though QUIC’s single-read path ignores `ERR_MSG_TOO_BIG` and continues. Can we avoid permanently latching `inner_read_closed_ = true` on recoverable datagram errors (`ERR_MSG_TOO_BIG`), and add a unit test verifying `Read()` error propagation across two hops after an inner read error occurs?
Done
weak_factory_.GetWeakPtr(), std::move(data), packet_tos));When `download_throttle_` is set and a valid zero-length UDP datagram arrives (`result == 0`), passing result to `download_throttle_->RequestBytes(0, ...)` triggers an immediate runtime `CHECK_GT(bytes, 0)` crash in `bandwidth_throttle.cc:107`. Can `ProcessInnerReadResult()` special-case zero-byte packets (`if (result == 0)`) and enqueue them with the normal propagation delay (`OnDownloadThrottleGranted(std::move(data), packet_tos)`) without calling `RequestBytes()`?
Done
int n = std::min(dest_len, static_cast<int>(front.data.size()));When a queued datagram (`front.data.size()`) is larger than the caller's buffer (`dest_len`), `CopyFrontPacketInto()` truncates the datagram to `dest_len`, copies only the truncated bytes, pops `receive_queue_`, and returns `dest_len` as a successful read. POSIX returns `ERR_MSG_TOO_BIG` on truncation (`udp_socket_posix.cc:1141`), and Windows maps `WSAEMSGSIZE` to `ERR_MSG_TOO_BIG`; either way, callers should not receive a successful truncated datagram. If `static_cast<int>(front.data.size()) > dest_len`, can `CopyFrontPacketInto()` return `ERR_MSG_TOO_BIG` and consume/drop the queued datagram (`receive_queue_.pop_front();`) in the same way UDP consumes an oversized datagram from the kernel receive buffer?
Done
optional before production wiring: While `UDPSocketWin::ReadMultiple` also executes `NOTREACHED()`, `QuicProxyDatagramClientSocket::ReadMultiple` migrated to `ReadMultipleEmulator` (`quic_proxy_datagram_client_socket.cc:304`) specifically to avoid process crashes when QUIC's `QuicUseReadMultiple` feature (`kQuicUseReadMultiple`) is active (`crbug.com/515333601`). Before wiring this wrapper into production QUIC client flows, can we delegate to `ReadMultipleEmulator` or implement batching through the delay queue?
Done
When `send_queue_` saturates (`size() >= kMaxQueuedSendPackets`) and `!inner_write_pending_`, calling `wrapped_socket_->Write()` directly bypasses `send_queue_`. If this direct `wrapped_socket_->Write()` returns `ERR_IO_PENDING`, `Write()` returns `ERR_IO_PENDING` without setting `inner_write_pending_ = true`. When `send_timer_` later fires (`DrainOneWireSend()`), `!inner_write_pending_` passes and `DrainOneWireSend()` issues a second `wrapped_socket_->Write(...)` while the direct write is still pending, causing `UDPSocketPosix` / `UDPSocketWin` to crash on `CHECK(write_callback_.is_null())` (`udp_socket_posix.cc:594`, `udp_socket_win.cc:528`). Can we either set `inner_write_pending_ = true` when line 428 pends asynchronously (and wrap `callback` to clear it and resume draining without dropping caller completion), or consistently drop overflowing packets by returning `buffer_len` when `send_queue_` is full?
Returning buffer_len
When `Write()` is called in shaping mode (`config_.rtt.is_positive() || upload_throttle_`) before a successful `Connect*()` or when disconnected, `Write()` enqueues `PendingSend` and returns `buffer_len` synchronously without checking connection state. When `DrainOneWireSend()` later writes the packet to `wrapped_socket_`, `wrapped_socket_->Write()` returns `ERR_SOCKET_NOT_CONNECTED`. Because `OnInnerWireSendComplete()` (`:575`) silently drops post-sync errors, the disconnection failure is lost. Can we track connected state in `DelayedDatagramSocket` across successful `Connect*()` / async connect completion and `Close()`, then reject shaped writes with `ERR_SOCKET_NOT_CONNECTED` when not connected?
Done
if (send.dscp != DSCP_NO_CHANGE || send.ecn != ECN_NO_CHANGE) {optional: `PendingSend` snapshots the raw `DSCP_NO_CHANGE` / `ECN_NO_CHANGE` sentinels, then `DrainOneWireSend()` resolves them later against whatever TOS state the inner socket happens to have after earlier queued sends. That can differ from the app-effective TOS at `Write()` time, especially for partial sentinels like `SetTos(DSCP_NO_CHANGE, explicit_ecn)`. Can `DelayedDatagramSocket` maintain an effective DSCP/ECN cache (`effective_dscp_` / `effective_ecn_`) and resolve `NO_CHANGE` sentinels against the cached effective state when constructing `PendingSend` in `Write()`, so `DrainOneWireSend()` applies the exact intended TOS per packet?
Done
nit: In `OverflowRestoresPerPacketStateWhenInnerIdle` (`line 1374`), the return value of `socket->Write(...)` is discarded on the overflow path. Checking `EXPECT_EQ(socket->Write(...), 8);` ensures overflow return code assumptions are explicitly verified.
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan added 6 comments![]( "Open in Gerrit")
Thanks. Just one correctness issue around TOS emulation, a couple of test coverage requests and a pair of documentation nits. Otherwise it's looking really good.
// pipeline. A native batched implementation can replace this later.nit: Two scope claims are stale: shaped not-connected Writes are now rejected before success, and `GetPeerAddress()` cannot distinguish a valid zero-length datagram from EOF—it only reports the socket association. Can these bullets be corrected?
// queue-overflow error visible to QUIC.nit: This still describes the previous direct-write overflow behavior; The current CL drops the packet and reports the byte count. Please update this block and the unittest section header.
if (rv != ERR_IO_PENDING && rv == OK) {The ordinary `Connect()` acceptance path is tested, but this synchronous `ConnectAsync()` branch is not. This is the path QUIC takes because `UDPClientSocket::ConnectAsync()` completes synchronously; removing this assignment makes every subsequent shaped Write fail with `ERR_SOCKET_NOT_CONNECTED` while the current tests still pass. Can the existing connect tests verify a shaped Write after both a sync-completing `ConnectAsync()` and the async completion path?
DscpAndEcn packet_tos = wrapped_socket_->GetLastTos();This snapshot is required when read-ahead ingests packet B before packet A is delivered, but current GetLastTos coverage only checks the default before reads and after Close. Can we read two packets with distinct TOS values and assert GetLastTos after each delivery?
buffer, buffer_len, max_message_size, std::move(callback));`ReadMultipleEmulator` currently emits `.tos = 0` after both synchronous and asynchronous `Read()` completion. QUIC consumes `DatagramMetadata::tos` for ECN, so `kQuicUseReadMultiple` makes every packet appear NotECT despite this wrapper’s per-packet TOS snapshot. Can the emulator preserve `socket_->GetLastTos()` here? All call sites pass `DatagramClientSocket` instances, although the `DscpAndEcn` result will need encoding back into the raw TOS byte. Please also cover two distinct TOS values through ReadMultiple.
Related details
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 5 comments![]( "Open in Gerrit")
// pipeline. A native batched implementation can replace this later.nit: Two scope claims are stale: shaped not-connected Writes are now rejected before success, and `GetPeerAddress()` cannot distinguish a valid zero-length datagram from EOF—it only reports the socket association. Can these bullets be corrected?
Done
nit: This still describes the previous direct-write overflow behavior; The current CL drops the packet and reports the byte count. Please update this block and the unittest section header.
Done
The ordinary `Connect()` acceptance path is tested, but this synchronous `ConnectAsync()` branch is not. This is the path QUIC takes because `UDPClientSocket::ConnectAsync()` completes synchronously; removing this assignment makes every subsequent shaped Write fail with `ERR_SOCKET_NOT_CONNECTED` while the current tests still pass. Can the existing connect tests verify a shaped Write after both a sync-completing `ConnectAsync()` and the async completion path?
Done
DscpAndEcn packet_tos = wrapped_socket_->GetLastTos();This snapshot is required when read-ahead ingests packet B before packet A is delivered, but current GetLastTos coverage only checks the default before reads and after Close. Can we read two packets with distinct TOS values and assert GetLastTos after each delivery?
Done
buffer, buffer_len, max_message_size, std::move(callback));`ReadMultipleEmulator` currently emits `.tos = 0` after both synchronous and asynchronous `Read()` completion. QUIC consumes `DatagramMetadata::tos` for ECN, so `kQuicUseReadMultiple` makes every packet appear NotECT despite this wrapper’s per-packet TOS snapshot. Can the emulator preserve `socket_->GetLastTos()` here? All call sites pass `DatagramClientSocket` instances, although the `DscpAndEcn` result will need encoding back into the raw TOS byte. Please also cover two distinct TOS values through ReadMultiple.
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan added 11 comments![]( "Open in Gerrit")
Sorry for the delay, wanted to try to catch everything in one pass. Hopefully this is it.
class NET_EXPORT DelayedDatagramSocket : public DatagramClientSocket {Consider class-level sequence checker annotation. Consider adding thread sequence assertions across public interface operations to enforce Chromium threading invariants.
**Suggested Fix:** Add private member `SEQUENCE_CHECKER(sequence_checker_);` and check `DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);` across public methods (`Read`, `Write`, `Connect`, `Close`).
// task hop) — used as a surrogate for actual wire-arrival time. A caller Read()```suggestion
// task hop) - used as a surrogate for actual wire-arrival time. A caller Read()
```
#include <algorithm>Think this is unused.
```suggestion
```
CompletionOnceCallback callback) {`Read()` initiates inner read-ahead without checking `connected_`, and at line :479 the zero-RTT / unlimited-upload `Write()` path forwards directly without evaluating its connection check. Both entry points must synchronously return `ERR_SOCKET_NOT_CONNECTED` before accessing the inner socket. Otherwise, an unopened Windows UDP read path can trigger a release `CHECK` in underlying OS socket plumbing.
```suggestion
CompletionOnceCallback callback) {
if (!connected_) {
return ERR_SOCKET_NOT_CONNECTED;
}
```
Suggested Testing: In `delayed_datagram_socket_unittest.cc`, introduce a unit test fixture that instantiates a socket without calling `Connect()`, invokes `Read()`, `Write()`, and `ReadMultiple()`, and verifies `EXPECT_THAT(rv, IsError(ERR_SOCKET_NOT_CONNECTED))` without reaching underlying OS sockets.
callback) {```suggestion
callback) {
if (!connected_) {
return ERR_SOCKET_NOT_CONNECTED;
}
```
const NetworkTrafficAnnotationTag& traffic_annotation) {```suggestion
const NetworkTrafficAnnotationTag& traffic_annotation) {
if (!connected_) {
return ERR_SOCKET_NOT_CONNECTED;
}
```
// Inner-write errors are silently dropped: the application already saw a
// synchronous success from Write(), and UDP packet loss is a normal mode
// for the protocol. Continue draining the queue.
if (!send_queue_.empty()) {shaped `Write()` calls report synchronous success before network transmission, while `OnInnerWireSendComplete()` discards subsequent asynchronous wire failures. This behavior suppresses critical QUIC error handling for events such as `ERR_MSG_TOO_BIG` (MTU discovery) and persistent route failures.
```suggestion
if (result < 0) {
if (result == ERR_MSG_TOO_BIG && !on_mtu_error_callback_.is_null()) {
on_mtu_error_callback_.Run(result);
} else if (IsFatalSocketError(result)) {
connected_ = false;
send_queue_.clear();
latched_write_error_ = result;
return;
}
}
if (!send_queue_.empty()) {
```
return wrapped_socket_->SetTos(dscp, ecn);the wrapper snapshots metadata before the initial inner write, but discards the active `PendingSend` while that write is pending. A later QUIC packet can subsequently alter ECN state via `SetTos()`, causing POSIX or Windows nonblocking retries to transmit the earlier datagram using the later packet's metadata value.
**Possible Implementation:** Rather than destroying the popped packet object when `wrapped_socket_->Write()` returns `ERR_IO_PENDING`, retain the active transfer in an explicit `std::optional<PendingSend> in_flight_send_` member. When `SetTos()` is invoked, check whether `in_flight_send_` is active; if a write is pending or undergoing nonblocking retry, defer committing option mutations to `wrapped_socket_` until `OnInnerWireSendComplete()` finishes.
**Possible Testing:** Add a unit test where packet A is submitted under `SetTos(..., ECN_NO)` and returns `ERR_IO_PENDING`. While pending, configure `SetTos(..., ECN_CE)` for packet B, complete write A's callback, and verify via mock inspection that packet A hits the wire with `ECN_NO` and packet B with `ECN_CE`.
if (dscp != DSCP_NO_CHANGE) {
effective_dscp_ = dscp;
}
if (ecn != ECN_NO_CHANGE) {
effective_ecn_ = ecn;
}
tos_configured_ = true;
return wrapped_socket_->SetTos(dscp, ecn);```suggestion
int rv = wrapped_socket_->SetTos(dscp, ecn);
if (rv == OK) {
if (dscp != DSCP_NO_CHANGE) {
effective_dscp_ = dscp;
}
if (ecn != ECN_NO_CHANGE) {
effective_ecn_ = ecn;
}
tos_configured_ = true;
}
return rv;
```
EcnCodePoint ecn) {```suggestion
EcnCodePoint ecn) {
CHECK_NE(dscp, DSCP_NO_CHANGE);
CHECK_NE(ecn, ECN_NO_CHANGE);
```
Related details
- Yoav Weiss (@Shopify)
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 7 comments![]( "Open in Gerrit")
class NET_EXPORT DelayedDatagramSocket : public DatagramClientSocket {Consider class-level sequence checker annotation. Consider adding thread sequence assertions across public interface operations to enforce Chromium threading invariants.
**Suggested Fix:** Add private member `SEQUENCE_CHECKER(sequence_checker_);` and check `DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);` across public methods (`Read`, `Write`, `Connect`, `Close`).
Done
// task hop) — used as a surrogate for actual wire-arrival time. A caller Read()```suggestion
// task hop) - used as a surrogate for actual wire-arrival time. A caller Read()
```
Done
Think this is unused.
```suggestion
```
Done
CompletionOnceCallback callback) {`Read()` initiates inner read-ahead without checking `connected_`, and at line :479 the zero-RTT / unlimited-upload `Write()` path forwards directly without evaluating its connection check. Both entry points must synchronously return `ERR_SOCKET_NOT_CONNECTED` before accessing the inner socket. Otherwise, an unopened Windows UDP read path can trigger a release `CHECK` in underlying OS socket plumbing.
```suggestion
CompletionOnceCallback callback) {
if (!connected_) {
return ERR_SOCKET_NOT_CONNECTED;
}
```Suggested Testing: In `delayed_datagram_socket_unittest.cc`, introduce a unit test fixture that instantiates a socket without calling `Connect()`, invokes `Read()`, `Write()`, and `ReadMultiple()`, and verifies `EXPECT_THAT(rv, IsError(ERR_SOCKET_NOT_CONNECTED))` without reaching underlying OS sockets.
Done
callback) {```suggestion
callback) {
if (!connected_) {
return ERR_SOCKET_NOT_CONNECTED;
}
```
Done
const NetworkTrafficAnnotationTag& traffic_annotation) {```suggestion
const NetworkTrafficAnnotationTag& traffic_annotation) {
if (!connected_) {
return ERR_SOCKET_NOT_CONNECTED;
}
```
Done
EcnCodePoint ecn) {```suggestion
EcnCodePoint ecn) {
CHECK_NE(dscp, DSCP_NO_CHANGE);
CHECK_NE(ecn, ECN_NO_CHANGE);
```
- Patrick Meenan
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. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 3 comments![]( "Open in Gerrit")
// Inner-write errors are silently dropped: the application already saw a
// synchronous success from Write(), and UDP packet loss is a normal mode
// for the protocol. Continue draining the queue.
if (!send_queue_.empty()) {shaped `Write()` calls report synchronous success before network transmission, while `OnInnerWireSendComplete()` discards subsequent asynchronous wire failures. This behavior suppresses critical QUIC error handling for events such as `ERR_MSG_TOO_BIG` (MTU discovery) and persistent route failures.
```suggestion
if (result < 0) {
if (result == ERR_MSG_TOO_BIG && !on_mtu_error_callback_.is_null()) {
on_mtu_error_callback_.Run(result);
} else if (IsFatalSocketError(result)) {
connected_ = false;
send_queue_.clear();
latched_write_error_ = result;
return;
}
}
if (!send_queue_.empty()) {
```
Done
return wrapped_socket_->SetTos(dscp, ecn);the wrapper snapshots metadata before the initial inner write, but discards the active `PendingSend` while that write is pending. A later QUIC packet can subsequently alter ECN state via `SetTos()`, causing POSIX or Windows nonblocking retries to transmit the earlier datagram using the later packet's metadata value.
**Possible Implementation:** Rather than destroying the popped packet object when `wrapped_socket_->Write()` returns `ERR_IO_PENDING`, retain the active transfer in an explicit `std::optional<PendingSend> in_flight_send_` member. When `SetTos()` is invoked, check whether `in_flight_send_` is active; if a write is pending or undergoing nonblocking retry, defer committing option mutations to `wrapped_socket_` until `OnInnerWireSendComplete()` finishes.
**Possible Testing:** Add a unit test where packet A is submitted under `SetTos(..., ECN_NO)` and returns `ERR_IO_PENDING`. While pending, configure `SetTos(..., ECN_CE)` for packet B, complete write A's callback, and verify via mock inspection that packet A hits the wire with `ECN_NO` and packet B with `ECN_CE`.
Dropped setting the TOS in the shaping path. See comment.
if (dscp != DSCP_NO_CHANGE) {
effective_dscp_ = dscp;
}
if (ecn != ECN_NO_CHANGE) {
effective_ecn_ = ecn;
}
tos_configured_ = true;
return wrapped_socket_->SetTos(dscp, ecn);```suggestion
int rv = wrapped_socket_->SetTos(dscp, ecn);
if (rv == OK) {
if (dscp != DSCP_NO_CHANGE) {
effective_dscp_ = dscp;
}
if (ecn != ECN_NO_CHANGE) {
effective_ecn_ = ecn;
}
tos_configured_ = true;
}
return rv;
```
Yoav Weiss (@Shopify)
Removed SetTOS entirely from the shaping path. See comment above
Related details
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |