Fixes the asynchttpserver example some more #16599
Conversation
I dislike this example a lot (busy looping for FDs to be closed is a very poor waste of resources) but at least with these changes it's a little bit better.
| 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) |
There was a problem hiding this comment.
is there a way to wait until a FD is closed to avoid a forced wait?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
potential solution: timotheecour#750
why is that busy looping? shouldn't poll only return when an event happened? in any case, see #16603 which shows that there's a bigger problem, |
Sure, but just because an event happened doesn't mean an FD was freed.
I'll let @Araq take a look. He should be the best to resolve this :) |
|
In fact, thinking about this some more. I'm thinking this should be resolved at the You even get an error code for this...
|
|
Making |
How so? This |
|
What if you know the request opens 4 files etc, then you can prevent |
|
I would rather have the request handler fail too. It would even compose well with other error handling. You can easily handle these errors in the request handler too. In addition, imagine you have other threads, now you'll need a lock around your call to |
I dislike this example a lot (busy looping for FDs to be closed is a very poor waste of resources) but at least with these changes it's a little bit better.
I dislike this example a lot (busy looping for FDs to be closed is a very poor waste of resources) but at least with these changes it's a little bit better.
I dislike this example a lot (busy looping for FDs to be closed is a very
poor waste of resources) but at least with these changes it's a little bit
better.