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

allow forwarding reusePort; use asynctools instead of fork now that PR was merged #278

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

allow forwarding reusePort; use asynctools instead of fork now that PR was merged #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a78a6f7
allow forwarding `reusePort`; use asynctools instead of fork now that…
timotheecour Jun 27, 2021
d365c17
bump version
timotheecour Jun 27, 2021
8ba1e1a
change version
timotheecour Jul 3, 2021
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
6 changes: 3 additions & 3 deletions jester.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ type
of RouteCode:
data: ResponseData

const jesterVer = "0.5.0"
const jesterVer = "0.5.1"

proc toStr(headers: Option[RawHeaders]): string =
return $newHttpHeaders(headers.get(@({:})))
Expand Down Expand Up @@ -498,10 +498,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)
Copy link
Owner

Choose a reason for hiding this comment

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

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):

jester/jester.nim

Line 417 in 1809237

reusePort: reusePort,
(I don't like having an Optional here).

Copy link
Contributor Author

@timotheecour timotheecour Jul 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@timotheecour timotheecour Jul 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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|off which user could set for unrelated reasons even if using jester with numThreads = 1)
  • the only tricky case is reusePort = false with numThreads > 1, and we can for now ignore reusePort = false in 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 with resusePort = false.

to which you seemed to have agreed yesterday:

okay, just go for it

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@timotheecour timotheecour Jul 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer to have reusePort set to true in Jester and same for httpbeast by default. Any reason not do this?

Copy link
Contributor Author

@timotheecour timotheecour Jul 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@timotheecour timotheecour Jul 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

)
else:
self.httpServer = newAsyncHttpServer(reusePort=self.settings.reusePort)
self.httpServer = newAsyncHttpServer(reusePort = self.settings.reusePort)
let serveFut = self.httpServer.serve(
self.settings.port,
proc (req: asynchttpserver.Request): Future[void] {.gcsafe, closure.} =
Expand Down
4 changes: 2 additions & 2 deletions jester.nimble
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Package

version = "0.5.0" # Be sure to update jester.jesterVer too!
version = "0.5.1" # Be sure to update jester.jesterVer too!
author = "Dominik Picheta"
description = "A sinatra-like web framework for Nim."
license = "MIT"
Expand All @@ -17,4 +17,4 @@ when not defined(windows):

task test, "Runs the test suite.":
exec "nimble install -y asynctools@#0e6bdc3ed5bae8c7cc9"
exec "nim c -r tests/tester"
exec "nim c -r tests/tester"