Add DelayedDatagramSocket: UDP socket with simulated network delay [chromium/src : main]
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. |
Patrick Meenan (Gerrit)
Patrick Meenan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Patrick Meenan
| Code-Review | +1 |
1 comment
Related details
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |