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

Fixes the asynchttpserver example some more #16599

Merged
merged 1 commit into from
Jan 7, 2021
Merged

Fixes the asynchttpserver example some more #16599

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
15 changes: 7 additions & 8 deletions lib/pure/asynchttpserver.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,28 @@ runnableExamples:
# This example will create an HTTP server on port 8080. The server will
# respond to all requests with a `200 OK` response code and "Hello World"
# as the response body. Run locally with:
# `nim doc --doccmd:-d:nimAsynchttpserverEnableTest --lib:lib lib/pure/asynchttpserver.nim`
# `nim doc --doccmd:-d:nimAsyncHttpServerEnableTest --lib:lib lib/pure/asynchttpserver.nim`
import asyncdispatch
if defined(nimAsynchttpserverEnableTest):
if defined(nimAsyncHttpServerEnableTest):
proc main {.async.} =
const port = 8080
var server = newAsyncHttpServer()
proc cb(req: Request) {.async.} =
echo (req.reqMethod, req.url, req.headers)
let headers = {"Date": "Tue, 29 Apr 2014 23:40:08 GMT",
"Content-type": "text/plain; charset=utf-8"}
let headers = {"Content-type": "text/plain; charset=utf-8"}
await req.respond(Http200, "Hello World", headers.newHttpHeaders())

echo "test this with: curl localhost:" & $port & "/"
server.listen Port(port)
server.listen(Port(port))
while true:
if server.shouldAcceptRequest():
await server.acceptRequest(cb)
else:
# too many concurrent connections, `maxFDs` exceeded
poll()
# wait 500ms for FDs to be closed
await sleepAsync(500)
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to wait until a FD is closed to avoid a forced wait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the ideal indeed. There is no way to do this out of the box, you could potentially get a good approximation if you add some code into each close proc that would then complete a Future to let code awaiting that future know that the FDs have been freed. But that assumes all your FDs are created within Nim, if you call into C you're screwed.

More and more, I'm having a hard time keeping an open mind to this "improvement". I'm curious whether other languages implement something similar. I've never seen somebody work around FD limits like this so this really smells like a bad idea to me still, especially now that it's needing so many fixes.

Copy link

@aboisvert aboisvert Jan 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I know this PR has already been merged but I figured I'd share my opinion (two cents) regardless.

IMO, the real pitfall here is trying to cope with too few FDs silently. If a system runs of out FDs (or any resource for that matter), it should somehow tell the administrator that it's operating beyond its capabilities and is no longer able to fulfill requests. (i.e. a simple log statement would help)

It's great that we're making httpserver more resilient, but in almost all occurrences this is still an undesirable situation and so should probably be communicated.

EDIT: Ooops I didn't realize this change was for the runnableExamples section, but still my comment applies to the serve() proc declared in asynchttpserver.nim, which still uses the poll() method. It seems these two should be consistent and include some logging for the out-of-FD situation. If it helps, LMK if you'd like me to submit a 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.

If you're interested in making a PR then I would encourage you to implement what I described below (#16599 (comment)). That is unless you disagree with it. Personally I don't think we should expose this mechanism in the way that we do at all. Asynchttpserver should just call accept and handle the errors it receives appropriately, and yes it should log when that happens.

Copy link
Member

Choose a reason for hiding this comment

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

potential solution: timotheecour#750


asyncCheck main()
runForever()
waitFor main()

import asyncnet, asyncdispatch, parseutils, uri, strutils
import httpcore
Expand Down