search

dom96 / httpbeast

A highly performant, multi-threaded HTTP 1.1 server written in Nim.
MIT License
450 stars 53 forks source link

httpbeast: option to set reusePort #47

Closed timotheecour closed 3 years ago

timotheecour commented 3 years ago

/cc @dom96 the next PR (https://github.com/dom96/jester/pull/278) will allow using this in jester

timotheecour commented 3 years ago

I guess we are relying on the OS complaining about a port being reused without reusePort enabled? Can we add a nicer error message for that case?

yes, I can make a separate PR to add context info (in particular port) in std/net.bindAddr; this can be done independently of this PR (the change belongs in stdlib)

To add more context to this PR, jester.newSettings sets reusePort = false, but this flag (until this PR) wasn't honored:

before PR:

suppose some service already runs in port 5000 (that you may be unaware of, or that might bind right before you launch your new service); then your run: nim r tests/example.nim it doesn't fail, but if you run curl http://0.0.0.0:5000, it may send requests to the 1st running server instead of the one you just created, which is not a sane default.

after PR:

it now honors reusePort = false (which is the default set in jester.newSettings, which is a good default), and nim r tests/example.nim will fail with:

/Users/timothee/git_clone/nim/jester/tests/example.nim(3) example
/Users/timothee/git_clone/nim/jester/jester.nim(497) serve
/Users/timothee/git_clone/nim/httpbeast/src/httpbeast.nim(488) run
/Users/timothee/git_clone/nim/httpbeast/src/httpbeast.nim(325) eventLoop
/Users/timothee/git_clone/nim/Nim_devel/lib/pure/net.nim(963) bindAddr
/Users/timothee/git_clone/nim/Nim_devel/lib/pure/includes/oserr.nim(94) raiseOSError
Error: unhandled exception: Address already in use [OSError]

it's easy to see by runnning nim r tests/example.nim in 2 tabs.

future work

I'd recommend changing the default in httpbeast too (matching the default in jester which is saner):

server.setSockOpt(OptReusePort, settings.reusePort.get(true))
=>
server.setSockOpt(OptReusePort, settings.reusePort.get(false))

(i can do this in this PR too but it'd be a breaking change so i'd recommend doing it in another PR instead; at least now, users have a choice)

timotheecour commented 3 years ago

@dom96 PTAL

raising when numThreads>1 and reusePort = false just causes more complications, so see PR for what I did instead.

dom96 commented 3 years ago

Sorry, I prefer for this stuff to be explicit. So I changed things.