option to not delay TCPServer socket creation #24115

tanmaykm · 2017-10-13T06:15:57Z

This change adds an option to TCPServer socket constructor to not delay creation of the socket. This will allow TCP servers to set socket options like SO_REUSEPORT before the socket is bound and listened on.

With early initialization:

tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl false &
[1] 23061
tan@tanlt ~/temp/soreuseport $ creating socket bound to 8010...
has reuseport
tcp_reuseport returned 0
listening...
accepting...

tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl false
creating socket bound to 8010...
has reuseport
tcp_reuseport returned 0
listening...
accepting...

But that is not possible with delayed initialization:

tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl true &
[1] 22977
tan@tanlt ~/temp/soreuseport $ creating socket bound to 8010...
has reuseport
tcp_reuseport returned -1
listening...
accepting...

tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl true
creating socket bound to 8010...
has reuseport
tcp_reuseport returned -1
listening...
ERROR: LoadError: listen: address already in use (EADDRINUSE)
Stacktrace:
 [1] uv_error at ./libuv.jl:68 [inlined]
 [2] #listen#369 at ./stream.jl:933 [inlined]
 [3] listen at ./stream.jl:933 [inlined]
 [4] run_server(::Bool) at /home/tan/temp/soreuseport/soreuseport.jl:13
 [5] include_relative(::Module, ::String) at ./loading.jl:533
 [6] include(::Module, ::String) at ./sysimg.jl:14
 [7] process_options(::Base.JLOptions) at ./client.jl:325
 [8] _start() at ./client.jl:391
in expression starting at /home/tan/temp/soreuseport/soreuseport.jl:22

My soreuseport.jl is:

function run_server(delay)
    println("creating socket bound to 8010...")
    s = Base.TCPServer(; delay=delay)
    if ccall(:jl_has_so_reuseport, Int32, ()) == 1
        println("has reuseport")
        rc = ccall(:jl_tcp_reuseport, Int32, (Ptr{Void},), s.handle)
        println("tcp_reuseport returned ", rc)
    else
        println("no reuseport")
    end
    bind(s, Base.InetAddr(ip"0.0.0.0", 8010))
    println("listening...")
    listen(s)
    println("accepting...")
    s1 = accept(s)
    println("accepted, closing...")
    close(s1)
    close(s)
    println("done")
end

run_server(parse(Bool, ARGS[1]))

This change adds an option to TCPServer socket constructor to not delay creation of the socket. This will allow TCP servers to set socket options like `SO_REUSEPORT` before the socket is bound and listened on. With early initialization: ``` tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl false & [1] 23061 tan@tanlt ~/temp/soreuseport $ creating socket bound to 8010... has reuseport tcp_reuseport returned 0 listening... accepting... tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl false creating socket bound to 8010... has reuseport tcp_reuseport returned 0 listening... accepting... ``` But that is not possible with delayed initialization: ``` tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl true & [1] 22977 tan@tanlt ~/temp/soreuseport $ creating socket bound to 8010... has reuseport tcp_reuseport returned -1 listening... accepting... tan@tanlt ~/temp/soreuseport $ julia ./soreuseport.jl true creating socket bound to 8010... has reuseport tcp_reuseport returned -1 listening... ERROR: LoadError: listen: address already in use (EADDRINUSE) Stacktrace: [1] uv_error at ./libuv.jl:68 [inlined] [2] #listen#369 at ./stream.jl:933 [inlined] [3] listen at ./stream.jl:933 [inlined] [4] run_server(::Bool) at /home/tan/temp/soreuseport/soreuseport.jl:13 [5] include_relative(::Module, ::String) at ./loading.jl:533 [6] include(::Module, ::String) at ./sysimg.jl:14 [7] process_options(::Base.JLOptions) at ./client.jl:325 [8] _start() at ./client.jl:391 in expression starting at /home/tan/temp/soreuseport/soreuseport.jl:22 ``` My `soreuseport.jl` is: ``` function run_server(delay) println("creating socket bound to 8010...") s = Base.TCPServer(; delay=delay) if ccall(:jl_has_so_reuseport, Int32, ()) == 1 println("has reuseport") rc = ccall(:jl_tcp_reuseport, Int32, (Ptr{Void},), s.handle) println("tcp_reuseport returned ", rc) else println("no reuseport") end bind(s, Base.InetAddr(ip"0.0.0.0", 8010)) println("listening...") listen(s) println("accepting...") s1 = accept(s) println("accepted, closing...") close(s1) close(s) println("done") end run_server(parse(Bool, ARGS[1])) ```

StefanKarpinski

LGTM. Needs docs and ideally a test even if only to test that the keyword is accepted, if not that it does something in particular.

StefanKarpinski · 2017-10-13T16:23:15Z

test/socket.jl

+ if ccall(:jl_has_so_reuseport, Int32, ()) == 1
+ @test 0 == ccall(:jl_tcp_reuseport, Int32, (Ptr{Void},), s.handle)
+ end
+end


Maybe test the positive case as well? (Unless that's already done somewhere else)

Yes, that is tested elsewhere as that's the default.

StefanKarpinski · 2017-10-13T16:23:59Z

base/socket.jl

+
+# Keyword arg "delay": if true, libuv delays creation of socket fd till bind.
+# It can be set to false if there is a need to set socket options before
+# further calls to `bind` and `listen`, e.g. `SO_REUSEPORT`.


I meant as part of the doc string for the TCPServer constructor, although a comment is better than nothing.

TCPServer is not exported, so I think this should be okay for now.

tanmaykm · 2017-10-16T05:30:03Z

Can this be merged now?
It will be good to have this backported to 0.6 as well.

aviks · 2017-10-25T13:18:16Z

Bump, anything preventing this from being merged?

aviks · 2017-11-06T23:51:48Z

@ararslan Any chance getting this into 0.6.2 please?

ararslan · 2017-11-06T23:55:07Z

Yes, will do.

tkelman · 2017-11-07T11:27:35Z

A new keyword argument is a feature, not a bugfix.

tanmaykm · 2017-11-07T13:32:10Z

TCPServer constructor is not exported, and there is no change in the default behavior.

tkelman · 2017-11-07T13:43:21Z

So? Shouldn't encourage writing code that works on 0.6.2 that wouldn't work on 0.6.1 unless there's a legitimate bug involved.

StefanKarpinski · 2017-11-07T15:27:19Z

Strictly, @tkelman is right, of course, but @tanmaykm is also right that this is a new option to an unexported type, which is about as mild as a feature gets. What's at stake here? What is this feature needed for? Is this effectively a bug fix?

aviks · 2017-11-07T22:27:28Z

So this feature is needed to run a Julia web server with real world load. Currently, a Julia web server can effectively only serve one connection at a time (even though the webserver is written with @async). This allows us to serve multiple connections on one machine. So in some ways this is crucial for realistic deploys.

The actual underlying feature (SO_REUSEPORT) has been in Julia (via libuv) for a long time. However, it cannot be used, due to an incorrect order of initialisation (to simplify things a bit). This PR changes the order of initialisation based on the keyword argument, thus allowing the feature to be actually used. So yes, this could be seen as a bugfix, since the feature has been added, but was unable to be used.

I think we can safely say that a new keyword argument is pretty low risk, there are bugfixes with much higher risk (as we've seen recently). Hence I would argue for its inclusion.

StefanKarpinski · 2017-11-07T22:43:58Z

Could we just change the incorrect initialization order and consider that a bugfix?

tkelman · 2017-11-07T23:22:31Z

Or carry a local copy of the modified version of the constructor, which should work correctly even on 0.6.0 or 0.6.1.

tanmaykm · 2017-11-08T01:42:25Z

To answer Stefan's question, I consider the keyword argument as the proper fix.

The bug was that Julia allowed only one way of constructing server socket (delayed), where no options could be set until it is listened on. While delayed creation is useful to avoid locking resources until use, we also need early creation to be able to set certain options.

Without the keyword we will have to make all sockets initialize early, which I think is worse.

tanmaykm · 2017-11-08T02:57:08Z

TCPServer() is an internal API. Exported APIs listen and listenany are the only ones that use this. The next step should be to have these exported APIs changed. I would agree with not backporting those to v0.6.

I would however argue for backporting this. It would allow packages that are willing to depend on this internal API in > v0.6.2 onwards to get this feature without calling a bunch of other internal APIs.

There is no impact on packages that choose to be compatible across v0.6. They can either not use reuseport at all and be oblivious of this change, or implement all of the constructor on their own.

tkelman · 2017-11-08T03:22:05Z

There shouldn't be such a thing as an API that only exists in 0.6.2, internal or otherwise. Feature freeze for release-0.6 was in February.

StefanKarpinski reviewed Oct 13, 2017

View reviewed changes

added docs and test

b821ad8

tanmaykm mentioned this pull request Oct 13, 2017

Multi-core server JuliaWeb/HTTP.jl#5

Closed

StefanKarpinski reviewed Oct 13, 2017

View reviewed changes

StefanKarpinski approved these changes Oct 13, 2017

View reviewed changes

tanmaykm mentioned this pull request Oct 16, 2017

allow port reuse on HTTP end JuliaWeb/JuliaWebAPI.jl#62

Merged

aviks added the backport pending 0.6 label Oct 16, 2017

StefanKarpinski merged commit 306c02c into JuliaLang:master Oct 25, 2017

ararslan removed the backport pending 0.6 label Apr 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

option to not delay TCPServer socket creation #24115

option to not delay TCPServer socket creation #24115

tanmaykm commented Oct 13, 2017

StefanKarpinski left a comment

StefanKarpinski Oct 13, 2017

tanmaykm Oct 13, 2017

StefanKarpinski Oct 13, 2017

tanmaykm Oct 13, 2017

tanmaykm commented Oct 16, 2017

aviks commented Oct 25, 2017

aviks commented Nov 6, 2017

ararslan commented Nov 6, 2017

tkelman commented Nov 7, 2017

tanmaykm commented Nov 7, 2017

tkelman commented Nov 7, 2017

StefanKarpinski commented Nov 7, 2017

aviks commented Nov 7, 2017

StefanKarpinski commented Nov 7, 2017

tkelman commented Nov 7, 2017

tanmaykm commented Nov 8, 2017

tanmaykm commented Nov 8, 2017

tkelman commented Nov 8, 2017

option to not delay TCPServer socket creation #24115

option to not delay TCPServer socket creation #24115

Conversation

tanmaykm commented Oct 13, 2017

StefanKarpinski left a comment

Choose a reason for hiding this comment

StefanKarpinski Oct 13, 2017

Choose a reason for hiding this comment

tanmaykm Oct 13, 2017

Choose a reason for hiding this comment

StefanKarpinski Oct 13, 2017

Choose a reason for hiding this comment

tanmaykm Oct 13, 2017

Choose a reason for hiding this comment

tanmaykm commented Oct 16, 2017

aviks commented Oct 25, 2017

aviks commented Nov 6, 2017

ararslan commented Nov 6, 2017

tkelman commented Nov 7, 2017

tanmaykm commented Nov 7, 2017

tkelman commented Nov 7, 2017

StefanKarpinski commented Nov 7, 2017

aviks commented Nov 7, 2017

StefanKarpinski commented Nov 7, 2017

tkelman commented Nov 7, 2017

tanmaykm commented Nov 8, 2017

tanmaykm commented Nov 8, 2017

tkelman commented Nov 8, 2017