Skip to content
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.

Sign up for GitHub

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

Jump to bottom

reusePort = false now default; honored regardless of numThreads #50

Closed
wants to merge 1 commit into from
Closed

reusePort = false now default; honored regardless of numThreads #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion httpbeast.nimble
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Package

version = "0.4.0"
version = "0.4.1"
author = "Dominik Picheta"
description = "A super-fast epoll-backed and parallel HTTP server."
license = "MIT"
Expand Down
28 changes: 16 additions & 12 deletions src/httpbeast.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@ type
domain*: Domain
numThreads: int
loggers: seq[Logger]
reusePort: bool
## controls whether to fail with "Address already in use".
## Setting this to false will raise when `threads` are on.
reusePort: bool ## controls whether to fail with "Address already in use".

HttpBeastDefect* = ref object of Defect

Expand All @@ -67,7 +65,7 @@ proc initSettings*(port: Port = Port(8080),
bindAddr: string = "",
numThreads: int = 0,
domain = Domain.AF_INET,
reusePort = true): Settings =
reusePort = false): Settings =
Settings(
port: port,
bindAddr: bindAddr,
Expand Down Expand Up @@ -311,6 +309,12 @@ proc updateDate(fd: AsyncFD): bool =
result = false # Returning true signifies we want timer to stop.
serverDate = now().utc().format("ddd, dd MMM yyyy HH:mm:ss 'GMT'")

proc newSocketAux(settings: Settings): owned(Socket) =
result = newSocket(settings.domain)
result.setSockOpt(OptReuseAddr, true)
result.setSockOpt(OptReusePort, settings.reusePort)
result.bindAddr(settings.port, settings.bindAddr)

proc eventLoop(params: (OnRequest, Settings)) =
let (onRequest, settings) = params

Expand All @@ -319,12 +323,7 @@ proc eventLoop(params: (OnRequest, Settings)) =

let selector = newSelector[Data]()

let server = newSocket(settings.domain)
server.setSockOpt(OptReuseAddr, true)
if compileOption("threads") and not settings.reusePort:
raise HttpBeastDefect(msg: "--threads:on requires reusePort to be enabled in settings")
server.setSockOpt(OptReusePort, settings.reusePort)
server.bindAddr(settings.port, settings.bindAddr)
let server = newSocketAux(settings)
server.listen()
server.getFd().setBlocking(false)
selector.registerHandle(server.getFd(), {Event.Read}, initData(Server))
Expand Down Expand Up @@ -477,12 +476,17 @@ proc run*(onRequest: OnRequest, settings: Settings) =
echo("Starting ", numThreads, " threads")
if numThreads > 1:
when compileOption("threads"):
var settings2 = settings
if not settings.reusePort and numThreads > 1:
let server = newSocketAux(settings)
close(server) # binds to a temporary socket to honor `reusePort = false`
settings2.reusePort = true
Comment on lines +480 to +483
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like this. It completely ignores the user's wishes. If I ask for reuse port to be set to false then I want it to be set to false, not for httpbeast to assume it knows best and set it to true anyway.

reusePort should be true by default and we shouldn't be doing this magic. You can just as easily do this "bind trick" without messing with what the user wants. Just check whether binding to the port works always, that will fix the problem you are fixing too.

Copy link
Contributor Author

@timotheecour timotheecour Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If I ask for reuse port to be set to false then I want it to be set to false, not for httpbeast to assume it knows best and set it to true anyway.

this will guarantee it'll fail, I can't find any use case for that; the way I implemented it works with sane behavior with both reusePort:true|false (and I thought we had agreed on that earlier).

You can add an additional enforceReusePortMultithread: bool flag if you want for users that don't want the override, but it's of little utility as it's a guaranteed exception raised.

What you're suggesting goes against the default set in stdlib, and users would have the wrong behavior by default (and would be subject to TOCTOU for numthreads=1)

Copy link
Contributor Author

@timotheecour timotheecour Jul 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'd be ok with the following if needded:
replace reusePort: bool by reusePort: ReusePort = rpOff with
type ReusePort = enum rpOff, rpOffStrict, rpOn
where default is same as in PR, and rpOffStrict causes reusePort=false and doesn't get reset by httpbeast.

Copy link
Owner

Choose a reason for hiding this comment

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

What you're suggesting goes against the default set in stdlib, and users would have the wrong behavior by default (and would be subject to TOCTOU for numthreads=1)

httpbeast is it's own http library, I don't see why it needs to be consistent with stdlib.

With this PR you are giving reusePort a completely different meaning. It's no longer a simple setting for whether SO_REUSE_PORT should be set, we shouldn't be assigning different meanings to simple flags to handle some edge cases. If you want to avoid TOCTOU then you can surely just set this flag to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the tristate i suggested would make it clear and un-ambiguous; possibly with better name type ReusePortPolicy

Copy link
Owner

Choose a reason for hiding this comment

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

people usually stick to defaults and this is the wrong default for reasons already discussed (pre-existing server intercepts client connections), and inconsistent with what jester has been using and stdlib's very own httpserver

But this is the right default for httpbeast because it relies on it for performance. This is only the wrong default because it allows other software to bind a socket on the same port, unfortunately that's the price you pay for performance and I would argue POSIX's design choices.

in which case would this bind to a dummy socket happen? a user setting needs to control whether fail-fast can happen or not (regardless of threads:on|off or numThreads = 1 or > 1). Which user setting is that in your proposal?

The dummy socket bind would happen only to detect that another instance of httpbeast (or another program) is running on that port.

Why do we need a user setting for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is the right default for httpbeast because it relies on it for performance

neither this PR nor any of the alternatives I've suggested has any impact (positive or negative) on performance.

Why do we need a user setting for this?

I may want to run multithread httpbeast on port 1234 and then another instance of the same program (or a different program) on the same port 1234, for eg for resilience or if I want N process times P threads instead of N*P threads. This should be up to user setting whether to fail-fast if a port is already taken.

Copy link
Owner

Choose a reason for hiding this comment

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

neither this PR nor any of the alternatives I've suggested has any impact (positive or negative) on performance.

Yes, but you're doing that by ignoring the flag. Like I said, that's not acceptable.

I may want to run multithread httpbeast on port 1234 and then another instance of the same program (or a different program) on the same port 1234, for eg for resilience or if I want N process times P threads instead of N*P threads. This should be up to user setting whether to fail-fast if a port is already taken.

Alright, fine so let's have another flag for this: failOnExistingPort or something.

Copy link
Contributor Author

@timotheecour timotheecour Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but you're doing that by ignoring the flag. Like I said, that's not acceptable.

the irony is that jester is ignoring this flag when not using httpbeast, so I don't see how neither the status quo nor your proposal is better. In any case, for the sake of moving on, I've opened an alternative PR, see #52 which uses failOnExistingPort, which should be followed by dom96/jester#281 (in jester) which forwards jester.reusePort=false to httpbeast.failOnExistingPort=true (IMO less clean than what I was doing and subject to TOCTOU when numThreads=1, unlike what I was doing in this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok here's a version I'm happy with: #53, avoids TOCTOU, avoids ignoring a reusePort flag, sane default.

var threads = newSeq[Thread[(OnRequest, Settings)]](numThreads)
for i in 0 ..< numThreads:
createThread[(OnRequest, Settings)](
threads[i], eventLoop, (onRequest, settings)
threads[i], eventLoop, (onRequest, settings2)
)
echo("Listening on port ", settings.port) # This line is used in the tester to signal readiness.
echo("Listening on port ", settings2.port) # This line is used in the tester to signal readiness.
joinThreads(threads)
else:
assert false
Expand Down