1426366 - Turn off TFO if we detect connection stalls

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P1)

Description

[Dragana Damjanovic [:dragana]]

I will try to detect stalls like the one explained in bug 1395494.
Similar stalls can happen with tls1.2 on some older versions of PaloAlto firewall (this bug is fixed the newer versions).

Comment 1

[Dragana Damjanovic [:dragana]]

If a connection that is using TFO(really sending data during the tcp handshake) and TLS is idle for about 10s, a firewall will delete all connection states and no more packet for that connection will go through.

To detect this:
 - when a new transaction is added to a connection, check if the connection has been idle for 10s (pref network.tcp.tcp_fastopen_http_check_for_stalls_only_if_idle_for). If yes, mark the connection for checking (mCheckNetworkStallsWithTFO = true)
 - when the transaction data has been written to the network set mLastRequestBytesSentTime.
 - if there is no response for 20s (pref network.tcp.tcp_fastopen_http_stalls_timeout), increase mFastOpenStallsCounter
 - if mFastOpenStallsLimit (pref network.tcp.tcp_fastopen_http_stalls_limit set to 3) is reached, disable TFO

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: bug_detect_stalls_with_tfo.patch

Comment 3

[u408661]

Comment on attachment 8938111 [details] [diff] [review]
bug_detect_stalls_with_tfo.patch

Review of attachment 8938111 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good, just a couple questions on the determination of a stall before r+ing.

::: netwerk/protocol/http/Http2Session.cpp
@@ +274,5 @@
>  
> +  uint32_t nextTick = UINT32_MAX;
> +  if (mCheckNetworkStallsWithTFO && mLastRequestBytesSentTime) {
> +    PRIntervalTime initialResponseDelta = now - mLastRequestBytesSentTime;
> +    if (initialResponseDelta > gHttpHandler->FastOpenStallsTimeout()) {

Should this be >=? I tend to think of the timeout as being hit as soon as the time is elapsed. Also, with just > we could potentially get in a state in the else where nextTick is 0.

@@ +387,5 @@
>    }
>  
>    mStreamIDHash.Put(aNewID, stream);
> +
> +  // if the TCP fast Open has need used and conection was idle for some time

nit: capitalize the I in 'if', and should 'need' be 'been'? (You can drop the 'the' before 'TCP', as well.)

@@ +388,5 @@
>  
>    mStreamIDHash.Put(aNewID, stream);
> +
> +  // if the TCP fast Open has need used and conection was idle for some time
> +  // we will be cautious and watch out for bug 1395494. 

nit: trailing whitespace

@@ +393,5 @@
> +  if (!mCheckNetworkStallsWithTFO && mConnection) {
> +    RefPtr<nsHttpConnection> conn = mConnection->HttpConnection();
> +    if (conn && (conn->GetFastOpenStatus() == TFO_DATA_SENT) &&
> +        gHttpHandler->CheckIfConnectionIsStalledOnlyIfIdleForThisAmountOfSeconds() &&
> +        IdleTime() > gHttpHandler->CheckIfConnectionIsStalledOnlyIfIdleForThisAmountOfSeconds()) {

Again, maybe |IdleTime() >=| here? Same reasoning as above.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +642,5 @@
>  
>      NS_ENSURE_ARG_POINTER(trans);
>      NS_ENSURE_TRUE(!mTransaction, NS_ERROR_IN_PROGRESS);
>  
> +    // if the TCP fast Open has need used and conection was idle for some time

Same nit here as in the Http2Session version of this code.

@@ +646,5 @@
> +    // if the TCP fast Open has need used and conection was idle for some time
> +    // we will be cautious and watch out for bug 1395494.
> +    if (mNPNComplete && (mFastOpenStatus == TFO_DATA_SENT) &&
> +        gHttpHandler->CheckIfConnectionIsStalledOnlyIfIdleForThisAmountOfSeconds() &&
> +        IdleTime() > gHttpHandler->CheckIfConnectionIsStalledOnlyIfIdleForThisAmountOfSeconds()) {

Same question on >= here as above.

@@ +1371,5 @@
>  
> +    // Check for the TCP Fast Open related stalls.
> +    if (mCheckNetworkStallsWithTFO && mLastRequestBytesSentTime) {
> +        PRIntervalTime initialResponseDelta = now - mLastRequestBytesSentTime;
> +        if (initialResponseDelta > gHttpHandler->FastOpenStallsTimeout()) {

Same >= question again :)

Comment 4

[Dragana Damjanovic [:dragana]]

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #3)
> Comment on attachment 8938111 [details] [diff] [review]
> bug_detect_stalls_with_tfo.patch
> 
> Review of attachment 8938111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good, just a couple questions on the determination of a
> stall before r+ing.
> 
> ::: netwerk/protocol/http/Http2Session.cpp
> @@ +274,5 @@
> >  
> > +  uint32_t nextTick = UINT32_MAX;
> > +  if (mCheckNetworkStallsWithTFO && mLastRequestBytesSentTime) {
> > +    PRIntervalTime initialResponseDelta = now - mLastRequestBytesSentTime;
> > +    if (initialResponseDelta > gHttpHandler->FastOpenStallsTimeout()) {
> 
> Should this be >=? I tend to think of the timeout as being hit as soon as
> the time is elapsed. Also, with just > we could potentially get in a state
> in the else where nextTick is 0.
> 

I agree, otherwise we will have a strange behavior. Thanks.

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_detect_stalls_with_tfo.patch

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8484ea3dea8b0c91160cceea71dc14a7c8942649

Comment 7

[u408661]

Comment on attachment 8938343 [details] [diff] [review]
bug_detect_stalls_with_tfo.patch

Review of attachment 8938343 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e288b04ea679
Detect http transaction stalls with TFO. r=hurley

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e288b04ea679