If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make sure we call OnSocketWritable if needed during TCP Fast Open

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

55 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 months ago
This is a work around for the problem explained in bug 1352156.
(Assignee)

Comment 1

5 months ago
Created attachment 8862868 [details] [diff] [review]
bug_0RTT_writable
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)
(Assignee)

Comment 3

5 months ago
Created attachment 8862969 [details] [diff] [review]
bug_writing0rttdata.patch

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)
(Assignee)

Comment 5

5 months ago
(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 :)
(Assignee)

Comment 6

5 months ago
Created attachment 8863673 [details] [diff] [review]
bug_writing0rttdata_v2.patch

This should work with or without fast open
Attachment #8862969 - Attachment is obsolete: true
Attachment #8863673 - Flags: review?(mcmanus)
(Assignee)

Comment 7

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a2633b54aa442b37cb02975077723ee4b41e97
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+

Comment 9

5 months ago
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

Comment 10

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/caf4f16f0437
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.