allow forwarding reusePort; use asynctools instead of fork now that PR was merged
#278
Conversation
| @@ -495,10 +495,10 @@ proc serve*( | |||
| proc (req: httpbeast.Request): Future[void] = | |||
| {.gcsafe.}: | |||
| result = handleRequest(jes, req), | |||
| httpbeast.initSettings(self.settings.port, self.settings.bindAddr) | |||
| httpbeast.initSettings(self.settings.port, self.settings.bindAddr, reusePort = self.settings.reusePort) | |||
There was a problem hiding this comment.
Argh, this is quite frustrating. For httpbeast reusePort should be set to true by default by Jester, but then this creates an inconsistency. I guess let's just make reusePort true by default for everything (even asynchttpserver):
Line 417 in 1809237
Optional here).
There was a problem hiding this comment.
the default in std/newAsyncHttpServer is reusePort = false; false is the correct default to avoid connections being silently forwarded to the wrong server; this has always been the case since the flag was introduced in nim in 2015; the default should remain false, like we discussed yesterday.
Then everything is consistent betweem std/newAsyncHttpServer, jester, and httpbeast.
For multithread, we can handle this in a second phase to also be consistent there, as dicussed yesterday.
There was a problem hiding this comment.
httpbeast needs to have this enabled by default and hence Jester does too. I don't know what you're suggesting, but if it's to just ignore the false value in httpbeast then I don't think that's right. If someone asks for no reusePort we should give it to them.
There was a problem hiding this comment.
I don't know what you're suggesting,
we discussed this yesterday:
https://discord.com/channels/371759389889003530/768367394547957761/860630931117965313
- reusePort is off by default (sane behavior, no impact on performance), so this is consistent with
std/newAsyncHttpServer - reusePort = true is easy to handle (regardless of numThreads)
- reusePort = false is easy to handle (with numThreads == 1, regardless of
--threads:on|offwhich user could set for unrelated reasons even if using jester with numThreads = 1) - the only tricky case is
reusePort = falsewith numThreads > 1, and we can for now ignorereusePort = falsein this case (noting it in docs, as i had done in my httpbeast PR), and in followup work (which I can volunteer to do) honor it as discussed yesterday, by first binding to that port (which would fail if in use) and then running jester threads withresusePort = false.
to which you seemed to have agreed yesterday:
okay, just go for it
There was a problem hiding this comment.
Yes, I know we discussed it yesterday. But I only now realised that this means we are going to ignore reusePort being set to false.
I would prefer to have reusePort set to true in Jester and same for httpbeast by default. Any reason not do this?
There was a problem hiding this comment.
I only now realised that this means we are going to ignore reusePort being set to false.
that was already the case before this PR, except it was worse because it was ignored for httpbeast but not for useHttpBeast = false; at least with this PR it's honored regardless of useHttpBeast:on|off, so long numThreads = 1
and in the suggested future work, it'll be honored in all cases.
There was a problem hiding this comment.
I would prefer to have reusePort set to true in Jester and same for httpbeast by default. Any reason not do this?
There was a problem hiding this comment.
The reason is reusePort = false is the saner default, and is also consistent with the setting in std/newAsyncHttpServer, so that when you start a server on an already bound port, you fail fast instead of succeeding and wondering why requests you send to it are not being processed by the server you just launched (or worse, if such requests are sensitive and should only talk to the server you launched).
There was a problem hiding this comment.
you'll already fail fast though via the bind trick though. This might not be the case for asynchttpserver for now (although we can add the same mechanism there as in httpbeast to make it so). I prefer speed by default.
There was a problem hiding this comment.
you'll already fail fast though via the bind trick though
We shouldn't be subject to TOCTOU if numThreads = 1 (regardless of --threads:on|off), and users shouldn't have to opt-in to get the sane default.
I prefer speed by default.
I don't see why it would impact speed in any way.
Here, I've finally implemented honoring reusePort=false for multithreads, see dom96/httpbeast#50. This should remove any remaining concerns.
timotheecour
force-pushed
the
pr_jester_fwd_reusePort
branch
from
1809237
to
8ba1e1a
Compare
to be merged after dom96/httpbeast#47 since it depends on it