1310197 - Implement TCP Fast Open in NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Dragana Damjanovic [:dragana]]

Linux use SendTo to send data during TCP Fast Open.
OSX uses connectx
and Windows uses ConnectEx.

I made the interface for all platforms to be the same as on Linux. So necko and nss will use PR_SendTo to send data during TCP Fast Open.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_nspr_v1.patch

Comment 2

[Dragana Damjanovic [:dragana]]

Comment on attachment 8801102 [details] [diff] [review]
bug_1310197_nspr_v1.patch

sorry, dropping the review flag, I want to make a small change.

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_nspr_v1.patch

Comment 4

[Patrick McManus [:mcmanus]]

Ted, we would really like to move forward with this patch. aiui it is additive and shouldn't really pose a regression risk. if you would like me to review it as well, I would be happy to do so. thanks!

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8803345 [details] [diff] [review]
bug_1310197_nspr_v1.patch

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

ted, I've read this code and believe it to be correct and 100% accretive (and therefore of low regression risk). it blocks another important feature - can we proceed on that basis?

Comment 6

[Dragana Damjanovic [:dragana]]

Comment on attachment 8803345 [details] [diff] [review]
bug_1310197_nspr_v1.patch

Daniel, can you look at this patch as well? Thanks!!!

Comment 7

[Daniel Stenberg [:bagder]]

Comment on attachment 8803345 [details] [diff] [review]
bug_1310197_nspr_v1.patch

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

Looks good. Just one question and some very minor nits.

::: nsprpub/pr/src/io/prsocket.c
@@ +1164,5 @@
>  {
>      *out_flags = 0;
> +#if defined(_WIN64)
> +    if (in_flags & PR_POLL_WRITE) {
> +        if (fd->secret->alreadyConnected) {

I couldn't see that 'alreadyConnected' is ever switched off again. Won't that make polling on this socket always return writable even after the connection has been established?

::: nsprpub/pr/src/md/windows/w95sock.c
@@ +436,5 @@
> +    // ConnectEx return TRUE on a success and FALSE on an error.
> +    if (_pr_win_connectex( (SOCKET)osfd, (struct sockaddr *) addr,
> +                           addrlen, buf, amount,
> +                           &rvSent, &fd->secret->ol) == TRUE)
> +    {

nit: brace moved to the previous line?

@@ +472,5 @@
> +            // this data already send.
> +            return amount;
> +        }
> +        while (err == ERROR_IO_PENDING)
> +        {

nit: open brace on the same line as "while" and the same comment for the three following if() clauses below.

::: nsprpub/pr/src/pthreads/ptio.c
@@ +2057,5 @@
> +#if !defined(DARWIN) || HAS_CONNECTX
> +    PRInt32 syserrno, bytes = -1;
> +    PRBool fNeedContinue = PR_FALSE;
> +    pt_SockLen addr_len;
> +        const PRNetAddr *addrp = addr;

indent mistake? (and one below)

@@ +2068,5 @@
> +
> +    PR_ASSERT(IsValidNetAddr(addr) == PR_TRUE);
> +#if defined(_PR_INET6)
> +        if (addr->raw.family == PR_AF_INET6) {
> +                md_af = AF_INET6;

nit: indent level hiccup

@@ +2125,5 @@
> +        bytes = pt_Continue(&op);
> +        syserrno = op.syserrno;
> +    }
> +    if (bytes < 0)
> +        pt_MapError(_PR_MD_MAP_SENDTO_ERROR, syserrno);

Use { braces } ?

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8803345 [details] [diff] [review]
> bug_1310197_nspr_v1.patch
> 
> Review of attachment 8803345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ted, I've read this code and believe it to be correct and 100% accretive
> (and therefore of low regression risk). it blocks another important feature
> - can we proceed on that basis?

Yes, if the networking peers are satisfied with this code that's good enough for me.

Comment 9

[Patrick McManus [:mcmanus]]

@ted - thanks.

when we've got an updated patch with an r+ from me and :bagder, let's commit to m-c and ni? ted to make sure an nspr release happens in parallel. thanks!

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to Daniel Stenberg [:bagder] from comment #7)
> Comment on attachment 8803345 [details] [diff] [review]
> bug_1310197_nspr_v1.patch
> 
> Review of attachment 8803345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Just one question and some very minor nits.
> 
> ::: nsprpub/pr/src/io/prsocket.c
> @@ +1164,5 @@
> >  {
> >      *out_flags = 0;
> > +#if defined(_WIN64)
> > +    if (in_flags & PR_POLL_WRITE) {
> > +        if (fd->secret->alreadyConnected) {
> 
> I couldn't see that 'alreadyConnected' is ever switched off again. Won't
> that make polling on this socket always return writable even after the
> connection has been established?
> 

Thanks!! Go catch. This will happen really rare; it can only happen if connectEx returns true, which means socket is connected.

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_nspr_v1.patch

rebased and fix Daniel's comments.

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_nspr_v1.patch

Update commit message.

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_nspr_part2.patch

This is needed for bug 1188435 comment 46. sndto will not be null but _PR_InvalidInt.
_PR_InvalidInt is private so doing it in more elegant way.

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8854951 [details] [diff] [review]
bug_1310197_nspr_part2.patch

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

That isn't a natural solution for nspr. how about

1] extern PRIntn _PR_InvalidInt(void); extern PRInt64 _PR_InvalidInt64(void); - ? Its kludgy, but we already do some kludgy nspr stuff and at least we'd fail at link time if something changed.

or

2] testing sendto() at startup time with something bogus, but checking for PR_INVALID_METHOD_ERROR as the response

Comment 15

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> Comment on attachment 8854951 [details] [diff] [review]
> bug_1310197_nspr_part2.patch
> 
> Review of attachment 8854951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That isn't a natural solution for nspr. how about
> 
> 1] extern PRIntn _PR_InvalidInt(void); extern PRInt64
> _PR_InvalidInt64(void); - ? Its kludgy, but we already do some kludgy nspr
> stuff and at least we'd fail at link time if something changed.
> 

I have tried that but it does not compile, it cannot find _PR_InvalidInt. I need to change the linking dependence but I do not know where that is :) I could try to figure it out, probably I need to expose _PR_InvalidInt or something


> or
> 
> 2] testing sendto() at startup time with something bogus, but checking for
> PR_INVALID_METHOD_ERROR as the response

not sure about this one. nspr will crash if you call sendto and that is not implemented. If I change that I am changing nspr interface and we do not want that.

Comment 16

[Patrick McManus [:mcmanus]]

(In reply to Dragana Damjanovic [:dragana] from comment #15)
> (In reply to Patrick McManus [:mcmanus] from comment #14)
> > Comment on attachment 8854951 [details] [diff] [review]
> > bug_1310197_nspr_part2.patch
> > 
> > Review of attachment 8854951 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > That isn't a natural solution for nspr. how about
> > 
> > 1] extern PRIntn _PR_InvalidInt(void); extern PRInt64
> > _PR_InvalidInt64(void); - ? Its kludgy, but we already do some kludgy nspr
> > stuff and at least we'd fail at link time if something changed.
> > 
> 
> I have tried that but it does not compile, it cannot find _PR_InvalidInt. I
> need to change the linking dependence but I do not know where that is :) I
> could try to figure it out, probably I need to expose _PR_InvalidInt or
> something
> 

yeah, that's true because nspr can live outside of libxul I suppose.

hmm.


> 
> > or
> > 
> > 2] testing sendto() at startup time with something bogus, but checking for
> > PR_INVALID_METHOD_ERROR as the response
> 
> not sure about this one. nspr will crash if you call sendto and that is not
> implemented. If I change that I am changing nspr interface and we do not
> want that.

I'm having flashbacks to seeing this before. it asserts, right? Which is kinda bizarre for doing feature detection.

Comment 17

[Patrick McManus [:mcmanus]]

The proposed patch doesn't solve the problem either the more I think about it.. if you need nspr v2 for sendto but only v1 is installed (the point of the patch) PR_SentToMethodPresent isn't going to be present either and some dynamic linker fail is going to happen :(

Comment 18

[Patrick McManus [:mcmanus]]

nsprpub/pr/tests/version.c seems to provide a guide to explicitly version testing.. its ugly but could work I suppose.

Comment 19

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #18)
> nsprpub/pr/tests/version.c seems to provide a guide to explicitly version
> testing.. its ugly but could work I suppose.

I do not why I have not think about it earlier. We have a couple of reserved_fn_* methods and they are all _PR_InvalidInt. I can compare sendto to them.

I have tried to check version using guideline from nsprpub/pr/tests/version.c, but it does not work. there is no libVersionPoint. I tried 2 different ways but it cannot find it in a nspr lib :(

Comment 20

[Patrick McManus [:mcmanus]]

sounds promising!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54d50e4a99b2
Implement fast open nspr part. r=mcmanus,bagder

Comment 22

[Dragana Damjanovic [:dragana]]

I have push this to mozilla-inbound.

Ted, we will need a nspr release.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4d5a3704d00366a598687552dec52f5f7de64c because it seems to have upset 64-bit windows:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=56e3c896e6cc81a36f613c3e64e066695d92718f&noautoclassify&group_state=expanded&filter-searchStr=win%20x64&tochange=1e4d5a3704d00366a598687552dec52f5f7de64c

Comment 24

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_nspr_v1.patch

Comment 25

[Dragana Damjanovic [:dragana]]

Comment on attachment 8858328 [details] [diff] [review]
bug_1310197_nspr_v1.patch

ConnectContinue and Poll was not correct.

Comment 26

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06a273c80bbf432633ee949ed824b1a66e64ce1e

Comment 27

[Daniel Stenberg [:bagder]]

Comment on attachment 8858328 [details] [diff] [review]
bug_1310197_nspr_v1.patch

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

Looks good to me, I only detected some minor nits.

::: nsprpub/pr/src/io/prsocket.c
@@ +318,5 @@
> +    if (fd->secret->overlappedActive) {
> +        PR_ASSERT(fd->secret->nonblocking);
> +        PRInt32 rvSent;
> +        if (GetOverlappedResult(osfd, &fd->secret->ol, &rvSent, FALSE) == TRUE)
> +        {

open brace at the end of the if () line?

::: nsprpub/pr/src/pthreads/ptio.c
@@ +2082,5 @@
> +#ifndef _PR_HAVE_SOCKADDR_LEN
> +        addrCopy = *addr;
> +        addrCopy.raw.family = AF_INET6;
> +        addrp = &addrCopy;
> +#endif

For the case when _PR_HAVE_SOCKADDR_LEN is *not* defined, the code copies the addr struct and sets the family field to INET6 only when the family is already set to PR_AF_INET6, while for the case when _PR_HAVE_SOCKADDR_LEN *is* defined it always copies the struct and sets the family field. This difference could use a little comment to explain it, since it isn't that easily spotted at a first glance.

@@ +2132,5 @@
> +        bytes = pt_Continue(&op);
> +        syserrno = op.syserrno;
> +    }
> +    if (bytes < 0)
> +        pt_MapError(_PR_MD_MAP_SENDTO_ERROR, syserrno);

Forgot { braces } for this conditional block.

@@ +3251,5 @@
>      pt_Shutdown,
>      pt_Recv,
>      pt_Send,
>      (PRRecvfromFN)_PR_InvalidInt,
> +    pt_TCP_SendTo, // Thiss is for TCP Fast Open. Linux uses SendTo function for this. OSX uses connectx, but we imitate Linux.

s/Thiss/this

Comment 28

[Patrick McManus [:mcmanus]]

Comment on attachment 8858328 [details] [diff] [review]
bug_1310197_nspr_v1.patch

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

::: nsprpub/pr/src/io/prsocket.c
@@ +1165,5 @@
> +
> +#if defined(_WIN64)
> +    if (in_flags & PR_POLL_WRITE) {
> +        if (fd->secret->alreadyConnected) {
> +            fd->secret->alreadyConnected = PR_FALSE;

let's clear this when we take action on the socket, not in level triggered poll

Comment 29

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86dd7e52005b

Comment 30

[Dragana Damjanovic [:dragana]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/919b9ff342f395acb9fbf82d79c809a226fba8a3
Bug 1310197 - Implement fast open nspr part. r=mcmanus,bagder

Comment 31

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_nspr_v1.patch

addressed comments.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/919b9ff342f3

Comment 33

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8859886 [details] [diff] [review]
bug_1310197_nspr_v1.patch

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

::: nsprpub/pr/src/pthreads/ptio.c
@@ +2094,5 @@
> +#endif
> +
> +#ifdef _PR_HAVE_SOCKADDR_LEN
> +    // if _PR_HAVE_SOCKADDR_LEN is defined and it is PR_AF_INET6 we set family
> +    / to AF_INET6 and we set address length.

It seems that it missed a slash and result in build-fail.

Comment 34

[Dragana Damjanovic [:dragana]]

Sorry, I will have a patch to fix it.

Comment 35

[Dragana Damjanovic [:dragana]]

Attachment: bug_1310197_fix_comment.patch

This fix a comment only!

Comment 36

[Dragana Damjanovic [:dragana]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b55ee79bb6892163f723eef325a217fb4b2842a
Bug 1310197 - Fix comment.r=me

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b55ee79bb68

Comment 38

[Kai Engert (:KaiE:)]

Unfortunately you haven't followed the NSPR project rules when committing these changes to the Mozilla tree.

NSPR is a separate product, with its own release cycle.

The nsprpub directory inside the Mozilla/Firefox tree must use released or beta snapshots of the NSPR project, only.
Direct commits into the mozilla/nsprpub aren't allowed.

The reason is that some consumers of Firefox, such as Linux distributions, package NSPR and NSS separately, and don't take the copy shipped as part of the Firefox sources, but use the released NSPR project archives for producing the system NSPR package. This means, if you don't follow the proper process, then those Linux distributions will miss the code that you have landed directly in the Firefox tree, and things will be really messed up...

All changes to NSPR must be checked in to the NSPR project tree at 
  https://hg.mozilla.org/projects/nspr/

Then you need to request that the NSPR/NSS group uplifts a new NSPR snapshot into mozilla-central.
The current bug for this process is bug 1350291.

(Hopefully we'll soon have a commit hook that prevents direct landings into the nsprpub directory, this is being worked on in bug 1348852.)

I'd like to kindly ask all reviewers to NSPR code to remind contributors of this process in the future.

Comment 39

[Kai Engert (:KaiE:)]

I've pushed the NSPR portions of your patch to the NSPR tree:
  https://hg.mozilla.org/projects/nspr/rev/f39f28732459374ed4fa5a5a5b5b03d81c5d5aac

Comment 40

[Dragana Damjanovic [:dragana]]

I know the rules, I have not push this change to nspr project yet, because I wanted the changes to be tested for a short time on nightly to see if everything is fine before pushing it to nspr project. The gecko changes that actually use nspr code landed last Friday

Comment 41

[Kai Engert (:KaiE:)]

Your commit was almost 3 weeks ago. It would have been helpful if you had mentioned the temporary nature of your commit in the commit message, this would have allowed me to identify your intentions. The commit messages in this bug also didn't give an easily to find clue.

Also, this bug had been marked resolved fixed, which suggested that the work on this is done. For a temporary experiment, I would have expected the bug to remain open until the experiment is completed and the final decisions have been made.

I would like to suggest, if you intend to repeat such experiments in the future, please announce them to the NSPR/NSS developers, to ensure that more people aware of the requirement to potentially backout local changes prior to a beta merge.

Comment 42

[Dragana Damjanovic [:dragana]]

Sorry for confusion. I would like to ship this in 55. I just have not push it into nspr project, because I was waiting to see if everything is fine, actually to test this code. And that took too long, I pushed gecko code that uses this nspr code only on Friday. I wanted to be sure that code is stable before pushing it to nspr project.

For now the NSPR part of the code for TCP fast open looks stable. I have not seen any crashes in there. So thank you for pushing it to nspr project repo. We can keep it and I hope everything is going to work just fine.

Comment 43

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie:) from comment #39)
> I've pushed the NSPR portions of your patch to the NSPR tree:
>  
> https://hg.mozilla.org/projects/nspr/rev/
> f39f28732459374ed4fa5a5a5b5b03d81c5d5aac

I had missed the change to "configure" when landing into NSPR,
this commit adds the missing piece:
  https://hg.mozilla.org/projects/nspr/rev/f869b8701568

Comment 44

[Mark Straver]

Sorry if this is a little unrelated and on a closed bug but I'm running into an issue upgrading to NSPR4.15 in my source tree when building Win 64-bit, which is directly related to the code added here. I need some help; maybe you can provide a pointer as to how to solve the following Win 64 build bustage?

 1:05.31 w95sock.obj : error LNK2019: unresolved external symbol WSAIoctl referenced in function _pr
_set_connectex
 1:05.31
 1:05.31 nss3.dll : fatal error LNK1120: 1 unresolved externals

Comment 45

[Kai Engert (:KaiE:)]

Mark, please file a new, separate bug on this issue, where you explain what software (that uses NSPR) you are trying you build, and CC the people who were active on this bug. In the new bug you could needinfo Dragana, as she had provided the patches, and might know best what new link options you require.

Comment 46

[Roy Tam]

Sorry for asking a dumb question here, but: from the code changed, TCP Fast Open seems to be available only to 64bit windows build and letting 32bit windows build untouched. Does it mean TCP Fast Open is only available in 64bit Windows?