1242464 - Call shutdown before closesocket and use "hard" close on Windows

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dragana Damjanovic [:dragana]]

We will call shutdown(socket) before calling closesocket.

We will also set Linger to hard close:
setting l_onoff to non zero and l_linger to zero:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms737582%28v=vs.85%29.aspx

https://msdn.microsoft.com/en-us/library/ms739165.aspx

Comment 1

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=082b2a8ba365

Comment 2

[Patrick McManus [:mcmanus]]

obviously nspr will eventually need to be updated, but that's a pita. Do you think we could have a hacky ifdef version of this in nssockettransport to validate it?

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> obviously nspr will eventually need to be updated, but that's a pita. Do you
> think we could have a hacky ifdef version of this in nssockettransport to
> validate it?

setoption probably can be done outside nspr, but shutdown will be really hacky, I think adding a layer above PR_NSPR_IO_LAYER could work

Comment 4

[Patrick McManus [:mcmanus]]

I think you can probably skip the layer and just do  PROsfd osfd = PR_FileDesc2NativeHandle(fd); in the hacky bit :)

Comment 5

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> I think you can probably skip the layer and just do  PROsfd osfd =
> PR_FileDesc2NativeHandle(fd); in the hacky bit :)

I thought about that, but I need to ask security layer is not going to attempt writing to the socket? Actually the file descriptor still exist, but it will just get an error, so that is safe ...

Comment 6

[Patrick McManus [:mcmanus]]

you're right that it might get in the way of a clean tls shutdown..

I guess the layer can be inserted right before the close is done tho..

Comment 7

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=692a711c385c

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_1242464_v1.patch

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1242464_v2.patch

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8711872 [details] [diff] [review]
bug_1242464_v2.patch

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

::: netwerk/base/ShutdownLayer.cpp
@@ +16,5 @@
> +
> +PRStatus
> +WinSockClose(PRFileDesc *aFd)
> +{
> +  PROsfd osfd = PR_FileDesc2NativeHandle(aFd);

can we check the return value?

@@ +18,5 @@
> +WinSockClose(PRFileDesc *aFd)
> +{
> +  PROsfd osfd = PR_FileDesc2NativeHandle(aFd);
> +  shutdown(osfd, SD_BOTH);
> +  return PR_SUCCESS;

still need to call lower->close, right?

::: netwerk/base/nsSocketTransport2.cpp
@@ +1360,5 @@
>  
> +#if defined(XP_WIN)
> +    // Turn the linger option on an set the interval to 0. This will cause hard
> +    // close of the socket.
> +    opt.option =  PR_SockOpt_Linger;

I haven't read up on the actual bytes involved.. is this the same as DONT_LINGER referenced by one article?

Comment 11

[Dragana Damjanovic [:dragana]]

> >  
> > +#if defined(XP_WIN)
> > +    // Turn the linger option on an set the interval to 0. This will cause hard
> > +    // close of the socket.
> > +    opt.option =  PR_SockOpt_Linger;
> 
> I haven't read up on the actual bytes involved.. is this the same as
> DONT_LINGER referenced by one article?

SO_DONTLINGER only manipulate l_onoff parameter but not l_linder. So if you use SO_DONTLINGER to set l_onoff to 1 l_linder is implementation dependent.

We want l_onoff to be nonzero and l_linder to be 0.

https://msdn.microsoft.com/en-us/library/ms739165.aspx

Comment 12

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d9fe2e674d0

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: bug_1242464_v2.patch

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8712082 [details] [diff] [review]
bug_1242464_v2.patch

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

I would want to see at least a hand test of a try build that exercises this code to make sure it doesn't somehow turn into "linger for some amount of time". The definitions seem so overlapping to me. do you think that's a good idea?

possibly - a local service that accepts a connection and just sits there not reading anything.

get firefox to connect and POST a few MB.. should sit in socket buffers after rwin is exhausted. call this close path in socket transport and measure it with a timestamp and a log.

::: netwerk/base/nsSocketTransport2.cpp
@@ +1359,5 @@
>      }
>  
> +#if defined(XP_WIN)
> +    // Turn the linger option on an set the interval to 0. This will cause hard
> +    // close of the socket.

I would even write more about how the default state is a little different. and include the link to https://msdn.microsoft.com/en-us/library/ms739165.aspx - that was helpful to me and this is a non intuitive bit of code

Comment 15

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c91a4373c9d

Comment 16

[Dragana Damjanovic [:dragana]]

Attachment: bug_1242464_v2.patch

rebased.

Comment 17

[Dragana Damjanovic [:dragana]]

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

Comment 18

[Dragana Damjanovic [:dragana]]

Attachment: bug_1242464_v2.patch

comment updated.

Comment 19

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> Comment on attachment 8712082 [details] [diff] [review]
> bug_1242464_v2.patch
> 
> Review of attachment 8712082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I would want to see at least a hand test of a try build that exercises this
> code to make sure it doesn't somehow turn into "linger for some amount of
> time". The definitions seem so overlapping to me. do you think that's a good
> idea?
> 
> possibly - a local service that accepts a connection and just sits there not
> reading anything.
> 
> get firefox to connect and POST a few MB.. should sit in socket buffers
> after rwin is exhausted. call this close path in socket transport and
> measure it with a timestamp and a log.
> 


I tried this did not work. I also tried the release version on the same computer, but i could not make socket buffer grow.
In test PR_Close always return immediately (0ms).

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b973ffb1e642

Comment 21

[Carsten Book [:Tomcat]]

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