1360570 - Make sure we call OnSocketWritable if needed during TCP Fast Open

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Dragana Damjanovic [:dragana]]

This is a work around for the problem explained in bug 1352156.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_0RTT_writable

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8862868 [details] [diff] [review]
bug_0RTT_writable

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

did you forget to include a file in the patch? This is the mechanism, but nothing seems to use it..

::: netwerk/base/nsSocketTransport2.h
@@ +197,5 @@
>          MSG_INPUT_CLOSED,
>          MSG_INPUT_PENDING,
>          MSG_OUTPUT_CLOSED,
> +        MSG_OUTPUT_PENDING,
> +        MSG_FORCE_OUTPUT_WRITE_DURING_0RTT

I think we would be better off adding
MSG_OUTPUT_READY and MSG_INPUT_READY rather than something specifically named for 0rtt.. but the impl would be the same (plus the mInput version)

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_writing0rttdata.patch

I am so sorry for the nice, I included a wrong patch.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8862969 [details] [diff] [review]
bug_writing0rttdata.patch

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

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +50,5 @@
>      // called by a transaction to resume either sending or receiving data
>      // after a transaction returned NS_BASE_STREAM_WOULD_BLOCK from its
>      // ReadSegments/WriteSegments methods.
>      //
> +    virtual MOZ_MUST_USE nsresult ResumeSend(bool aForceIOIf0RTT) = 0;

I guess you're going to have to explain why we need this parameter.. it doesn't look like it would spin any more than the level triggered poll/write (which pretty much ALWAYS fires).. and its a terribly hard things to understand what to set it at in an Abstract interface..

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ -1440,5 @@
>      return NS_ERROR_UNEXPECTED;
>  }
>  
> -
> -class HttpConnectionForceIO : public Runnable

forgot that one existed :)

Comment 5

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> Comment on attachment 8862969 [details] [diff] [review]
> bug_writing0rttdata.patch
> 
> Review of attachment 8862969 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsAHttpConnection.h
> @@ +50,5 @@
> >      // called by a transaction to resume either sending or receiving data
> >      // after a transaction returned NS_BASE_STREAM_WOULD_BLOCK from its
> >      // ReadSegments/WriteSegments methods.
> >      //
> > +    virtual MOZ_MUST_USE nsresult ResumeSend(bool aForceIOIf0RTT) = 0;
> 
> I guess you're going to have to explain why we need this parameter.. it
> doesn't look like it would spin any more than the level triggered poll/write
> (which pretty much ALWAYS fires).. and its a terribly hard things to
> understand what to set it at in an Abstract interface..

The reason for this is http2: if I do not add this parameter http2 will spin all the time. H2 has buffered some data and also had sent that data during 0rtt, so it does not have data to send any more but the buffer is not empty (we keep all the data that we have sent during 0rtt in the buffer). SetWriteCallbacks is called each time we call ReadSegments and ResumeSend will be called each time  as well.
If I change the condition at the line:
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/Http2Session.cpp#505
the session will not async wait for the socket at all :) 

Probably there is a different way to do this as well: maybe we should not call SetWriteCallbacks() every time we enter ReadSegments during 0RTT, i.e. do not call it if ReadSegment is called because of a ForseIO call and the session does not have new data to send. I do not know h2 code that well, but I will take a look.


> 
> ::: netwerk/protocol/http/nsHttpConnection.cpp
> @@ -1440,5 @@
> >      return NS_ERROR_UNEXPECTED;
> >  }
> >  
> > -
> > -class HttpConnectionForceIO : public Runnable
> 
> forgot that one existed :)

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_writing0rttdata_v2.patch

This should work with or without fast open

Comment 7

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a2633b54aa442b37cb02975077723ee4b41e97

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8863673 [details] [diff] [review]
bug_writing0rttdata_v2.patch

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

::: netwerk/protocol/http/Http2Session.cpp
@@ +502,5 @@
>  void
>  Http2Session::SetWriteCallbacks()
>  {
> +  if (mConnection &&
> +      (GetWriteQueueSize() || (mOutputQueueUsed > mOutputQueueSent))) {

that probably should have been that way anyhow..

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caf4f16f0437
Make sure to write as much data as possible during FastOpen. r=mcmanus

Comment 10

[Carsten Book [:Tomcat]]

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