Closed Bug 1360570 Opened 2 years ago Closed 2 years ago

Make sure we call OnSocketWritable if needed during TCP Fast Open

Categories

(Core :: Networking: HTTP, enhancement)

55 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 2 obsolete files)

This is a work around for the problem explained in bug 1352156.
Attached patch bug_0RTT_writable (obsolete) — Splinter Review
Attachment #8862868 - Flags: review?(mcmanus)
Attachment #8862868 - Attachment is patch: true
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)
Attachment #8862868 - Flags: review?(mcmanus)
Attached patch bug_writing0rttdata.patch (obsolete) — Splinter Review
I am so sorry for the nice, I included a wrong patch.
Attachment #8862868 - Attachment is obsolete: true
Attachment #8862969 - Flags: review?(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 :)
Attachment #8862969 - Flags: review?(mcmanus)
(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 :)
This should work with or without fast open
Attachment #8862969 - Attachment is obsolete: true
Attachment #8863673 - Flags: review?(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..
Attachment #8863673 - Flags: review?(mcmanus) → review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caf4f16f0437
Make sure to write as much data as possible during FastOpen. r=mcmanus
https://hg.mozilla.org/mozilla-central/rev/caf4f16f0437
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.