Skip to content

httpbeast: option to set reusePort #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

timotheecour
Copy link
Contributor

@timotheecour timotheecour commented Jun 27, 2021

/cc @dom96
the next PR (dom96/jester#278) will allow using this in jester

Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines 59 to 60
# processes bind to same address/port. We use an Option to avoid
# breaking code that relies on `Settings(port: port)`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I think we can live with this breakage. initSettings should be used anyway.

Copy link
Contributor Author

@timotheecour timotheecour Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's in the wild though... in fact, 1st version of this didn't use an Option and it broke some code. Other than that, i agree that initSettings should be used.

note that the recommended way (initSettings) remains clean (no Option used there), so if anything, this is an implementation detail.

@timotheecour
Copy link
Contributor Author

timotheecour commented Jul 1, 2021

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 timotheecour force-pushed the pr_httpbeast_reusePort branch from a29b94f to e08209b Compare July 2, 2021 00:02
@timotheecour
Copy link
Contributor Author

timotheecour commented Jul 2, 2021

@dom96 PTAL

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

dom96 added a commit that referenced this pull request Jul 2, 2021
Squashed commit of the following:

commit 43ee8c3
Author: Dominik Picheta <[email protected]>
Date:   Fri Jul 2 21:50:46 2021 +0100

    Fix `run`.

commit 804d42c
Author: Dominik Picheta <[email protected]>
Date:   Fri Jul 2 21:42:03 2021 +0100

    Update src/httpbeast.nim

commit eca38f9
Author: Dominik Picheta <[email protected]>
Date:   Fri Jul 2 21:40:22 2021 +0100

    Update src/httpbeast.nim

commit c316b87
Author: Dominik Picheta <[email protected]>
Date:   Fri Jul 2 21:36:33 2021 +0100

    Apply suggestions from code review

commit e08209b
Author: Timothee Cour <[email protected]>
Date:   Thu Jul 1 17:02:43 2021 -0700

    ignore reusePort when running with multiple threads for now

commit 64310f4
Author: Timothee Cour <[email protected]>
Date:   Sun Jun 27 15:42:52 2021 -0700

    fix tests (use an Option to make code backward compatible)

commit d6564d0
Author: Timothee Cour <[email protected]>
Date:   Sun Jun 27 14:52:39 2021 -0700

    bump version

commit 39b93cf
Author: Timothee Cour <[email protected]>
Date:   Wed Apr 17 10:09:08 2019 -0700

    httpbeast: option to set reusePort
@dom96
Copy link
Owner

dom96 commented Jul 2, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants