Permanent version of TCP Fast Open

RESOLVED FIXED in 4.18

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

other
4.18
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We have a temporary function expose to Firefox t o enabme canceling overlapped IOs.

This bug should make a permanent solution.
There where 2 opinions that I have heard while working on TFO:

1) Implement TFO in nspr
2) implement TFO outside nspr and do not change nspr


I think we want the first version because of nss.

I have a patch for the first version.

If someone objects on version 1 please comment.

Eric, Mike, Kai? (Eric has already expressed his opinion that we want version 1.)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(kaie)
Flags: needinfo?(ekr)
Well, if it's for nss... there's not much other choice. But if it's entirely for Gecko, then it's better not to change nspr.
Flags: needinfo?(mh+mozilla)
> Well, if it's for nss... there's not much other choice.

Actually... I'm puzzled, is gecko letting nss handle nspr sockets on its own? how does that reconcile with proxies?
(In reply to Mike Hommey [:glandium] from comment #3)
> > Well, if it's for nss... there's not much other choice.
> 
> Actually... I'm puzzled, is gecko letting nss handle nspr sockets on its
> own? how does that reconcile with proxies?

nss is only a layer (to be correct, it is multiple layer) between gecko and nspr. Gecko is handling sockets, creating, polling and closing them.
Posted patch bug_cancelIO_nspr_v1.patch (obsolete) — Splinter Review
This is a solution for option 1).
Attachment #8914723 - Flags: review?(kaie)
Attachment #8914723 - Flags: review?(honzab.moz)
Comment on attachment 8914723 [details] [diff] [review]
bug_cancelIO_nspr_v1.patch

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

::: nsprpub/pr/src/io/prio.c
@@ +151,5 @@
> +
> +  PR_Lock(_fd_waiting_for_overlapped_done_lock);
> +
> +  PRFileDescList *cur = _fd_waiting_for_overlapped_done;
> +  PRFileDescList *previous = NULL;

have you considered

PRFileDescList **cur = &_fd_waiting_for_overlapped_done;

instead of using the |previous| bit?

::: nsprpub/pr/src/md/windows/w95thred.c
@@ +68,5 @@
> +            PR_Unlock(_fd_waiting_for_overlapped_done_lock);
> +            if (cur ) {
> +                PR_Sleep(delay); // wait another 5s.
> +            }
> +        } while (cur);

please have a limit of iterations.  I think overall it should not take more than 10-15s.  The |delay| should be 1s or less.  Then I would leak, including the lock.  Probably only doable in release builds?
Attachment #8914723 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Comment on attachment 8914723 [details] [diff] [review]
> bug_cancelIO_nspr_v1.patch
> 
> ::: nsprpub/pr/src/md/windows/w95thred.c
> @@ +68,5 @@
> > +            PR_Unlock(_fd_waiting_for_overlapped_done_lock);
> > +            if (cur ) {
> > +                PR_Sleep(delay); // wait another 5s.
> > +            }
> > +        } while (cur);
> 
> please have a limit of iterations.  I think overall it should not take more
> than 10-15s.  The |delay| should be 1s or less.  Then I would leak,
> including the lock.  Probably only doable in release builds?


I will change delay to 1s.

I wanted to limit the iterations, but i was not sure how. I wanted to open another bug for this. Some application using nspr do not want to limit it.
Actually we can discuss this here:

do we want to expose a function to set the max iteration time? This will be windows specific that is not prefered.
do we want to set this under #ifdef and define on undefine a macro at compile time.
Blocks: 1407301
Comment on attachment 8914723 [details] [diff] [review]
bug_cancelIO_nspr_v1.patch

Thank you for removing the temporary code.

I cannot review this code for correctness, but Honza's review is sufficient for that.

You're only changing code for the Windows platform, which I personally don't worry much about, because there are probably only few consumers of NSPR besides the Mozilla applications.

Nevertheless, you're changing code that will affect all consumers of NSPR on Windows. I don't know if your code could cause any surprises or regressions for them.

If you've decided that you create general code at the NSPR level, you probably should have tests inside the NSPR test suite, that verify this code is correct, or at least, that it doesn't cause regressions to those who don't use your new mechanism.

Although NSPR has its own test suite, it's currently not being executed anywhere. A good place to do so is probably the NSS CI environment. I've recently started some preprations to make that happen, see bug 1385039, but it's not yet completed. It would be nice to get some help there.

Lacking automated tests, please run the NSPR tests locally on your Windows machine.

r=kaie because you aren't introducing any new APIs, you aren't changing existing APIs, so your change is safe from the API perspective.
Flags: needinfo?(kaie)
Attachment #8914723 - Flags: review?(kaie) → review+
Also, I wish we'd be able to make progress with bug 1399095. We should be able to test experimental patches, prior to checking them in.
Attachment #8914723 - Attachment is obsolete: true
Attachment #8929488 - Flags: review+
This patch will not hurt legacy application on Windows because it can only be triggered by using SendTo on a TCP socket which does not make sense except for TCP Fast Open and the function was implemented when TFO was added.
Also the ConnectEx is dynamically loaded, so if windows version is too old if will detect that it does not exist and do nothing. The part of the code that uses ConnectEx is already in nspr for more than 6 months.
https://hg.mozilla.org/projects/nspr/rev/1572f7b21403
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.18
Flags: needinfo?(ekr)
Blocks: 1421247
Duplicate of this bug: 1394808
You need to log in before you can comment on or make changes to this bug.