[Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets

[Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets

[Timur Safin]

Using SO_LINGER over unix sockets makes no much sense, though
it's harmless on Linux. The problem is, it breaks majority of 
tests under Windows/WSL with assertion, because setsockopt()
would return EINVAL in that case under WSL.

This is known WSL issue and reported here 
https://github.com/microsoft/WSL/issues/3992

So we filter out SO_LINGER if evio_setsockopt_server is being
called with AF_UNIX family.
 
Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4659-wsl-no-linger-assert

 
---
 src/lib/core/evio.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/lib/core/evio.c b/src/lib/core/evio.c
index 2152c15e6..06aa11ce4 100644
--- a/src/lib/core/evio.c
+++ b/src/lib/core/evio.c
@@ -140,13 +140,16 @@ evio_setsockopt_server(int fd, int family, int type)
 		       &on, sizeof(on)))
 		return -1;
 
-	/* Send all buffered messages on socket before take
-	 * control out from close(2) or shutdown(2). */
-	struct linger linger = { 0, 0 };
+	if (family != AF_UNIX) {
+		/* Send all buffered messages on socket before
+		 * take control out from close(2) or shutdown(2). */
+		struct linger linger = { 0, 0 };
+
+		if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
+				&linger, sizeof(linger)))
+			return -1;
+	}
 
-	if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
-		       &linger, sizeof(linger)))
-		return -1;
 	if (type == SOCK_STREAM && family != AF_UNIX &&
 	    evio_setsockopt_keepalive(fd) != 0)
 		return -1;
-- 
2.20.1

Re: [Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 7 comments below.

1. Please, add a subsystem prefix to the commit title. For
examples see other commits in the repository.

On 10/03/2020 15:25, Timur Safin wrote:
> Using SO_LINGER over unix sockets makes no much sense, though
> it's harmless on Linux. The problem is, it breaks majority of 
> tests under Windows/WSL with assertion, because setsockopt()
> would return EINVAL in that case under WSL.> 
> This is known WSL issue and reported here 
> https://github.com/microsoft/WSL/issues/3992
> 
> So we filter out SO_LINGER if evio_setsockopt_server is being
> called with AF_UNIX family.
>  
> Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4659-wsl-no-linger-assert

2. Please, provide both branch and issue links.

3. I see, that on the branch your commit message is just empty.
Seems like you didn't push the latest message.

4. The patch has nothing to do with gh-4659:
https://github.com/tarantool/tarantool/issues/4659
"sql: raise an error in case space features HASH index".

> ---

5. Links should be below this marker '---'. Some people apply
patches from emails, and when you write links above '---', you
make them part of the commit message.

>  src/lib/core/evio.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/lib/core/evio.c b/src/lib/core/evio.c
> index 2152c15e6..06aa11ce4 100644
> --- a/src/lib/core/evio.c
> +++ b/src/lib/core/evio.c
> @@ -140,13 +140,16 @@ evio_setsockopt_server(int fd, int family, int type)
>  		       &on, sizeof(on)))
>  		return -1;
>  
> -	/* Send all buffered messages on socket before take
> -	 * control out from close(2) or shutdown(2). */
> -	struct linger linger = { 0, 0 };
> +	if (family != AF_UNIX) {

6. Is there any proof that it is no-op on Linux for AF_UNIX?

I would rather call sio_setsockopt() always. And ignore an
error, if it is EINVAL for AF_UNIX.

7. It is worth adding a comment why SO_LINGER is workarounded
somehow.

> +		/* Send all buffered messages on socket before
> +		 * take control out from close(2) or shutdown(2). */
> +		struct linger linger = { 0, 0 };
> +
> +		if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
> +				&linger, sizeof(linger)))
> +			return -1;
> +	}
>  
> -	if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
> -		       &linger, sizeof(linger)))
> -		return -1;
>  	if (type == SOCK_STREAM && family != AF_UNIX &&
>  	    evio_setsockopt_keepalive(fd) != 0)
>  		return -1;
> 

Re: [Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets

[Timur Safin]

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [PATCH] Work-around WSL assert when SO_LINGER is set on unix
: sockets
: 


: 
: 1. Please, add a subsystem prefix to the commit title. For
: examples see other commits in the repository.

Here I need a help, because evidence is not apparent. The last commit to 
evio.c was not marked with subsystem 

	tsafin@M1BOOK6319:~/tarantool$ git log --oneline src/lib/core/evio.c
	835d22aa0 (HEAD -> tsafin/gh-4659-wsl-no-linger-assert, master)
		Work-around WSL assert when SO_LINGER is set on unix sockets
	3f5f59bb5 Move 'core' and 'uuid' libs to src/lib

Should it be `core` or `coio`?

: > Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4659-wsl-
: no-linger-assert
: 
: 2. Please, provide both branch and issue links.
: 4. The patch has nothing to do with gh-4659:
: https://github.com/tarantool/tarantool/issues/4659
: "sql: raise an error in case space features HASH index".

This is the question - what is the current practice for such 
simplistic patches? SOP says there is no need to open issue 
so I'm marking branch with github issue # of which this 
patch is byproduct. 

:: 3. I see, that on the branch your commit message is just empty.
: Seems like you didn't push the latest message.

Yup, I didn't fully realize that such single-patch patchsets 
should contain patch commit messages and not that separate messages 
which I'd put to the cover message of patchset. Will amend.
 
: 
: > ---
: 
: 5. Links should be below this marker '---'. Some people apply
: patches from emails, and when you write links above '---', you
: make them part of the commit message.

Yes, thanks for the correction, this is side-effect of an approach
used above.
 
: > -	/* Send all buffered messages on socket before take
: > -	 * control out from close(2) or shutdown(2). */
: > -	struct linger linger = { 0, 0 };
: > +	if (family != AF_UNIX) {
: 
: 6. Is there any proof that it is no-op on Linux for AF_UNIX?
:

Good question! Never had enough time and motivation to proof this assumption.
Now you've asked I'll look into linux kernel tcp implementation. Stay ktuned.
 
 
: I would rather call sio_setsockopt() always. And ignore an
: error, if it is EINVAL for AF_UNIX.

Probably it's less damaging approach. I'll probably use it - but first I need to
Look around in the kernel.


: 
: 7. It is worth adding a comment why SO_LINGER is workarounded
: somehow.

Indeed. Will do .

: 
: > +		/* Send all buffered messages on socket before
: > +		 * take control out from close(2) or shutdown(2). */
: > +		struct linger linger = { 0, 0 };
: > +
: > +		if (sio_setsockopt(fd, SOL_SOCKET, SO_LINGER,
: > +				&linger, sizeof(linger)))
: > +			return -1;
: > +	}


Timur

Re: [Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets

[Cyrill Gorcunov]

On Wed, Mar 11, 2020 at 01:43:16PM +0300, Timur Safin wrote:
>  
> : > -	/* Send all buffered messages on socket before take
> : > -	 * control out from close(2) or shutdown(2). */
> : > -	struct linger linger = { 0, 0 };
> : > +	if (family != AF_UNIX) {
> : 
> : 6. Is there any proof that it is no-op on Linux for AF_UNIX?

Yes. The SO_LINGER setup interal SOCK_LINGER socket flag which
is used in inet streamed sockets (and a few other places).
For unix sockets it is not used.

Still this approach is somehow fragile. As to me such specifics
should either handled by #ifdef which would recognize WSL or
we need some bootstrap runtime testing (which will simply be
more confusing).

IOW, I propose to detect WSL on cmake level and #ifdef the things
which are not supported.

Re: [Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets

[Vladislav Shpilevoy]

Thanks for the answers!

On 11/03/2020 11:43, Timur Safin wrote:
> : -----Original Message-----
> : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> : Subject: Re: [PATCH] Work-around WSL assert when SO_LINGER is set on unix
> : sockets
> : 
> 
> 
> : 
> : 1. Please, add a subsystem prefix to the commit title. For
> : examples see other commits in the repository.
> 
> Here I need a help, because evidence is not apparent. The last commit to 
> evio.c was not marked with subsystem 
> 
> 	tsafin@M1BOOK6319:~/tarantool$ git log --oneline src/lib/core/evio.c
> 	835d22aa0 (HEAD -> tsafin/gh-4659-wsl-no-linger-assert, master)
> 		Work-around WSL assert when SO_LINGER is set on unix sockets
> 	3f5f59bb5 Move 'core' and 'uuid' libs to src/lib
> 
> Should it be `core` or `coio`?

I think neither of them. It should be 'evio'.

> : > Branch: https://github.com/tarantool/tarantool/tree/tsafin/gh-4659-wsl-
> : no-linger-assert
> : 
> : 2. Please, provide both branch and issue links.
> : 4. The patch has nothing to do with gh-4659:
> : https://github.com/tarantool/tarantool/issues/4659
> : "sql: raise an error in case space features HASH index".
> 
> This is the question - what is the current practice for such 
> simplistic patches? SOP says there is no need to open issue 
> so I'm marking branch with github issue # of which this 
> patch is byproduct. 

I am not sure I understand what is 'patch byproduct'.

If there is no issue, then you can omit issue link. But it certainly
is not right to take some random other issue number into the branch
name. Just omit the issue number everywhere.

    tsafin/wsl-no-linger-assert

instead of

    tsafin/gh-4659-wsl-no-linger-assert

> : > -	/* Send all buffered messages on socket before take
> : > -	 * control out from close(2) or shutdown(2). */
> : > -	struct linger linger = { 0, 0 };
> : > +	if (family != AF_UNIX) {
> : 
> : 6. Is there any proof that it is no-op on Linux for AF_UNIX?
> :
> 
> Good question! Never had enough time and motivation to proof this assumption.
> Now you've asked I'll look into linux kernel tcp implementation. Stay ktuned.

The question wasn't really about kernel sources. Rather about
documentation or standards, on which you could rely, when assume,
that SO_LINGER works for some socket types, and does not for
others.

I don't think we can omit SO_LINGER in case it is not guaranteed
that it is no-op for unix sockets.

However, we can omit it for WSL specifically, like Cyrill proposed.
Using cmake to add a new macro, and do nothing when macro says we
compile for WSL.

> : I would rather call sio_setsockopt() always. And ignore an
> : error, if it is EINVAL for AF_UNIX.
> 
> Probably it's less damaging approach. I'll probably use it - but first I need to
> Look around in the kernel.

Re: [Tarantool-patches] [PATCH] Work-around WSL assert when SO_LINGER is set on unix sockets

[Timur Safin]

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Subject: Re: [PATCH] Work-around WSL assert when SO_LINGER is set on unix
: sockets
: 


: >
: > Here I need a help, because evidence is not apparent. The last commit to
: > evio.c was not marked with subsystem
: >
: > 	tsafin@M1BOOK6319:~/tarantool$ git log --oneline src/lib/core/evio.c
: > 	835d22aa0 (HEAD -> tsafin/gh-4659-wsl-no-linger-assert, master)
: > 		Work-around WSL assert when SO_LINGER is set on unix sockets
: > 	3f5f59bb5 Move 'core' and 'uuid' libs to src/lib
: >
: > Should it be `core` or `coio`?
: 
: I think neither of them. It should be 'evio'.
: 

Ok, will use it for the next incarnation of reworked patch. 

: 
: If there is no issue, then you can omit issue link. But it certainly
: is not right to take some random other issue number into the branch
: name. Just omit the issue number everywhere.
: 
:     tsafin/wsl-no-linger-assert
: 
: instead of
: 
:     tsafin/gh-4659-wsl-no-linger-assert
: 

Ok, will use such naming schema


: > Good question! Never had enough time and motivation to proof this
: assumption.
: > Now you've asked I'll look into linux kernel tcp implementation. Stay
: ktuned.
: 
: The question wasn't really about kernel sources. Rather about
: documentation or standards, on which you could rely, when assume,
: that SO_LINGER works for some socket types, and does not for
: others.
: 
: I don't think we can omit SO_LINGER in case it is not guaranteed
: that it is no-op for unix sockets.
: 
: However, we can omit it for WSL specifically, like Cyrill proposed.
: Using cmake to add a new macro, and do nothing when macro says we
: compile for WSL.

Yup, dropped this patch already, and will pass down to sources something 
like WSL1_WORKAROUND defined. It should work in the nearest time-frame. 
WSL2 will go out of Insider Rings this April/May, so this workaround
should be very short-living. WSL2 is using full Linux kernel (patched by 
Azure guys) so it will have the same tcp socket implementation, and 
work-around will be disabled then.
 
Take care,
Timur