Summary:
We originally added it for onNetworkDdata only e.g.
https://fburl.com/code/lub7h950, but we should do it on sending pings as well
Reviewed By: mjoras
Differential Revision: D63670960
fbshipit-source-id: ad6096ddc92f00669239bcbb37e65d48238020b9
Summary: This case was missing. If we don't do this then if we buffer a packet on EAGAIN while we don't have initia/handshake data to write, we will loop on the write callback and do empty write loops.
Reviewed By: kvtsoy
Differential Revision: D63492172
fbshipit-source-id: 94ac1c37b2015d38694b9aa3be2744c9bbbe6bee
Summary:
fixes the arg passed to the ::onBidirectionalStreamsAvailable callback
#facebook:
i accidentally introduced a bad operator precedence bug in a refactor; this fixes that and the unit test
luckily HQSession doesn't consume the argument passed to the callback
Reviewed By: knekritz
Differential Revision: D62765762
fbshipit-source-id: 7ae206d5036fc744485736f92fd366ce99103d58
Summary: Delay writes when networkDataPerSocketRead=true until the read loop is done
Reviewed By: mjoras
Differential Revision: D62302229
fbshipit-source-id: 858bed523b5a46bd52fe71f6f0e8b23f5dd4e46d
Summary: Presently updateWriteLooper will always schedule a loop callback to do the write loop. Add an option to inline trigger the writes after the read event.
Reviewed By: kvtsoy, sharmafb
Differential Revision: D62192192
fbshipit-source-id: 30991608f57fbfa097a7522c0731d25b8e528345
Summary: Missed this call because processCallbacksAfterNetworkData() used to call it before
Reviewed By: mjoras
Differential Revision: D61891333
fbshipit-source-id: 078e9dce5e5f91f389bdb38b0a589a3256dd873b
Summary: The prior `while (maxOffsetToDeliver.has_value()) {` was misleading because `maxOffsetToDeliver` doesn't change in the body of the while loop. I'm changing the structure so as to make it more intuitive as to what's happening here.
Reviewed By: hanidamlaj
Differential Revision: D61801508
fbshipit-source-id: fb91b183316b281cf74cbb33a67f7080e7d8a6f8
Summary: I'm adding the software rx timestamp to `PacketsReceivedEvent` so that it's available in `SocketObserverInterface::packetsReceived`.
Reviewed By: mjoras
Differential Revision: D61485649
fbshipit-source-id: 05170acbcfd1fc8e6a2fce4d2cc8f3f2f7441134
Summary: This gives an option to process application callbacks per received and processed packet, rather than after a whole batch has been received.
Reviewed By: kvtsoy
Differential Revision: D61626616
fbshipit-source-id: cac434adff79eda738c2c4924a0080ecdaac1a25
Summary: We have a lot of optionals that are either integral values or std::chrono::microseconds. These end up wasting memory, where we can instead store sentinel values to encode whether the value is there or not. This reduces the effective range of the type by one value, but that is an acceptable tradeoff.
Reviewed By: kvtsoy
Differential Revision: D57684368
fbshipit-source-id: b406b86011f9b8169b6e5e925265f4829928cc63
Summary:
The idea here is to make it so we can swap out the type we are using for optionality. In the near term we are going to try swapping towards one that more aggressively tries to save size.
For now there is no functional change and this is just a big aliasing diff.
Reviewed By: sharmafb
Differential Revision: D57633896
fbshipit-source-id: 6eae5953d47395b390016e59cf9d639f3b6c8cfe
Summary:
Adds a transport setting for the dscp value. The transport combines this value with the ECN flags to enable ECT0/ECT1 marking.
Previous use-cases that are setting dscp values by overriding the whole ToS field should still work, but they will override the ECN bits. This is still the default behavior. Applications willing to use ECN, should use the dscp transport setting instead of applying the tos socket option directly.
Reviewed By: mjoras
Differential Revision: D58203586
fbshipit-source-id: 7dd83ca82273fadd4ae03b015387143e02101b6c
Summary:
There are still some timestamps seen on dynamic content/connections and disabling timestamps per hostname doesn't scale well.
Instead, this diff will disable timestamps by default in Liger and enable only for static content connections based on a hostname filter (`scontent`, `video`, `cdninstagram`). The actual meat of the change is in `AdvancedHTTPSessionManager::shouldEnableQuicAckRxTimestamps` - everything else is mostly a replacement of disable to enable.
This is implemented for MNS in D58099124.
Reviewed By: jbeshay
Differential Revision: D58099125
fbshipit-source-id: 06118163fcc5d2e2ce90028810d6a9af5c7958a9
Summary: We can throw in `writeSocketDataAndCatch()` and that would result in transport closure with drain (with drain keeping the socket alive), but the congestion controller would be reset in close: https://fburl.com/code/zxwfye5u and `maybeStopWriteLooperAndArmSocketWritableEvent()` trips over it later when executed in scope exit.
Reviewed By: mjoras
Differential Revision: D57728369
fbshipit-source-id: 51a4719ae97fab0e90e3ae093a3f56be5a096aff
Summary:
The existing batch writers do not handle failed writes to the AsyncUDPSocket. A packet that fails to be written is detected as a packet loss later when feedback is received from the peer. This negatively impacts the congestion controller because of the fake loss signal, and artificially inflates the number of retransmitted packets/bytes.
This change adds a new batch writer (SinglePacketBackpressuretBatchWriter) that retains the buffers when a write fails. For subsequent writes, the writer retries the same buffer. No new packets are scheduled until the retried buffer succeeds.
Notes:
- To make sure that retry writes are scheduled, the write callback is installed on the socket when a buffer needs to be retried.
- The retries are for an already scheduled packet. The connection state reflects the timing of the first attempt. This could still have an impact on rtt samples, etc. but it this is a milder impact compared to fake losses/retranmissions.
- Any changes outside of the batch writer only impact the new batch writer. Existing batch writers do not use the fields and are not affected by the changes in this diff.
Reviewed By: kvtsoy
Differential Revision: D57597576
fbshipit-source-id: 9476d71ce52e383c5946466f64bb5eecd4f5d549
Summary: Use writable events on the socket (disabled by default)
Reviewed By: jbeshay
Differential Revision: D56305786
fbshipit-source-id: f04dea326587268c96915f7a39338ff21dee4aec
Summary: Adding another WriteCallback in the next diff
Reviewed By: jbeshay
Differential Revision: D56371737
fbshipit-source-id: 4918ca2af3e49968ca0822d7a99309a05021fd10
Summary:
This adds validation to verify that packets sent with an ECT(0/1) marks, are being echoed correctly by the client without any interference in the network path. If validation succeeds, the transport can use the ECN feedback in the congestion controller. If it fails, packet marking is disabled.
Ideally, the number of marks echoed by the peer in ACK_ECN frames should be equal to the number of packets sent with marks. However, that is not achievable with the current design due to the following restrictions:
- To be able to track the number of marks accurately, the sending transport needs to match incoming acks with the outstanding packets they are acking. However, the sending transport's list of outstanding packets only tracks retransmittable packets. Therefore, acks containing marks that refer to non-retranmittable packets cannot be verified.
- If the client is using GRO, we have one header for the whole batch. So echoed marking can be inflated.
To work around these limitations, the validation logic checks that:
- At least 10 retransmittable packets were sent with marks in the AppData packet number space.
- The number of echoed marks is greater than the number of retransmittable AppData packets sent with marks.
- The number of echoed marks is less than the total number of packets sent.
- No unexpected marks show up. I.e. No ECT0 when sending ECT1 or vv.
Reviewed By: kvtsoy
Differential Revision: D55618562
fbshipit-source-id: 8bc44b4f1b64725f51f63bb86d6c4bd573338e83
Summary: Adds new field `tosValue` to ReceivedUdpPacket so it is accessible in the rest of the read path.
Reviewed By: kvtsoy
Differential Revision: D54912161
fbshipit-source-id: ea4714fa2374d38e915fc850387e1094d1fb8adf
Summary: ReceivedUdpPacket has attached metadata (currently timings and later in the stack, tos values). Rather than passing the metadata separately in the read path for the client and the server, this propagates the ReceivedUdpPacket further up so this metadata is easier to use in the rest of the stack.
Reviewed By: mjoras
Differential Revision: D54912162
fbshipit-source-id: c980d5b276704f5bba780ac16a380bbb4e5bedad
Summary:
Adds new transport settings for enabling quic sockets to read or write the tos/tclass field in the IP headers.
The new transport settings are: readEcnOnIngress, enableEcnOnEgress, useL4sEcn. All are defaulted to false.
readEcnOnIngress=true --> enabled reading the ToS field from incoming packets.
enableEcnOnEgress=false --> does not set marking
enableEcnOnEgress=true, useL4sEcn=false --> sets marking to ECT0
enableEcnOnEgress=true, useL4sEcn=true --> sets marking to ECT1
The next changes handle these packets in the rest of the stack.
Reviewed By: mjoras, kvtsoy
Differential Revision: D54877773
fbshipit-source-id: af6aefc714e2678f488d027583cf666200748782
Summary: I want to have visibility in to the current count of streams. There are already callback methods onNew/onClosed, unfortunately, the latter is not called when connections are closed, so I had to fix that to prevent the metric from uncontrolled growth. I am still not positive it's correct and was thinking of hooking into QuicStreamState destructor instead.
Reviewed By: kvtsoy
Differential Revision: D56195487
fbshipit-source-id: 901bce9473327a8f7ef1bc0f70af10899784d681
