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
Summary:
We currently do not make use of socket writeable events in mvfst (will be working on that soon), so we may end up in a situation where mvfst has pending data to write, but needs evb loop wake up to do so.
This change introduces a (immediate) timeout to wake up the loop right after it exits to write out the data we have left.
Alternative would be to make the packets per loop limit higher, but this _may_ be nicer given we do yield the loop for other things before waking it up again.
Reviewed By: jbeshay, mjoras
Differential Revision: D56224059
fbshipit-source-id: 2fc0f5000096def459c5d0cb98f6421d36ebbda4
Summary: Need to create buf accessor for it to work
Reviewed By: jbeshay, sharmafb
Differential Revision: D55264376
fbshipit-source-id: b58de662468f388ac78f369031626567a56988d9
Summary: - `swapStreams` is likely mis-implemented and causing an unnecessary copy of the "dst" vector
Reviewed By: mjoras
Differential Revision: D53139321
fbshipit-source-id: adbf1417ee46f10ca4b32389eae7ddaca873c790
Summary:
The current open connection counting is broken. To fix it this change:
- moves the counter to be for both the server and client transport
- calls onNewConnection() when the transport is ready.
- calls onConnectionClose() when the transport is closed after it was ready.
Reviewed By: lnicco, frankfeir
Differential Revision: D52638628
fbshipit-source-id: bcff7a8c671f8eede53c22e698e1b95332c56959
Summary:
This moves all the loop and timer callback operations back to the callbacks instead of relying the QuicEventBase to perform them. I.e. it's now Callback::cancelCallback() instead of QuicEventBase::cancelCallback(&callback).
To simplify the design, the lifetime of LoopCallbackWrapper and TimerCallbackWrapper has been changed. Previously these wrappers lasted for one iteration of the loop or the timer callback. In the new design the wrapper is created the first time the callback is scheduled and is only destroyed when the callback is destroyed. This significantly simplifies the lifetime management of the wrapper and the code overall.
While transitioning LibevQuicEventBase to this new design, this change also consolidates the QuicTimer functionality in the LibevQuicEventBase itself.
Reviewed By: hanidamlaj, mjoras
Differential Revision: D52217879
fbshipit-source-id: da7b95d592650ddc112813fdc3b9f5010d32e7fb
Summary: This is basically as a safeguard, to ensure that the drain timeout doesn't try to keep running after the destruction of the transport.
Reviewed By: jbeshay
Differential Revision: D52267494
fbshipit-source-id: d9ba99e499de7d01281b56bec40298f4e52e6fca
