The whole asynchttpserver server crashes when it has problem in one client connection. Could it be made more robust?
The server has the serve proc, with the following code:
proc serve*(server: AsyncHttpServer, port: Port,
callback: proc (request: Request): Future[void] {.closure, gcsafe.},
address = "";
assumedDescriptorsPerRequest = -1) {.async.} =
listen server, port, address
while true:
if shouldAcceptRequest(server, assumedDescriptorsPerRequest):
var (address, client) = await server.socket.acceptAddr()
asyncCheck processClient(server, client, address, callback)
else:
poll()
The problem is this line asyncCheck processClient(server, client, address, callback) if connection socket is killed and processClient can't do IO it crashes and asyncCheck crashes the whole Nim process via the runForever().
Wouldn't it be better to use some safe async wrapper log warning to console, instead of crashing the whole server?
Also, if such approach used, could there be some connection/socket/memory leaks?
Like code below:
proc processClientSafely(server: AsyncHttpServer, client: AsyncSocket, address: string,
callback: proc (request: Request):
Future[void] {.closure, gcsafe.}) {.async.} =
try:
processClient(server, client, address, callback)
except Exception as e:
echo e.msg
proc serve*(server: AsyncHttpServer, port: Port,
callback: proc (request: Request): Future[void] {.closure, gcsafe.},
address = "";
assumedDescriptorsPerRequest = -1) {.async.} =
listen server, port, address
while true:
if shouldAcceptRequest(server, assumedDescriptorsPerRequest):
var (address, client) = await server.socket.acceptAddr()
asyncCheck processClientSafely(server, client, address, callback)
else:
poll()
For my own programs I have created an asyncLog proc which does exactly what asyncCheck does, except it logs errors to terminal.
We have a number of options here:
Thanks for feedback.
Make what asyncCheck does overridable, so that we don't need to change all our asyncCheck calls.
Yes, that sounds like good option, possible implementation, note the new checkRequest parameter:
proc serve*(server: AsyncHttpServer, port: Port,
callback: proc (request: Request): Future[void] {.closure, gcsafe.},
address = "";
assumedDescriptorsPerRequest = -1,
checkRequest: proc (future: Future[void]): void = asyncCheck) {.async.} =
listen server, port, address
while true:
if shouldAcceptRequest(server, assumedDescriptorsPerRequest):
var (address, client) = await server.socket.acceptAddr()
checkRequest processClient(server, client, address, callback)
else:
poll()
For my own programs I have created an asyncLog proc which does exactly what asyncCheck does, except it logs errors to terminal.
I did similar thing proc spawn_async*[T](future: Future[T], check = true) it's copy of the asyncCheck with the check parameter that controls if error will be raised or the future result be ignored. I called it spawn because conceptually we are starting a separate control flow, a separately running program, async/future is just an implementation detail.
Eliminate the need for asyncCheck.
I think what asyncCheck does is a reasonable thing. We spawn separate program (as async) and we want to know how it's doing. We may extend it to do more, like sometimes we want to crash the whole program if one of its spawned children fails, sometimes we want to ignore it, sometimes we want to ignore and log it, sometimes spawn another if it fails. I think those use cases are too specific and should be handled by developers by providing its own checkAndDoWhatIWant function.
This is handled in the chronos http server like so: https://github.com/status-im/nim-chronos/blob/master/chronos/apps/http/httpserver.nim#L597-L606
In general, asyncCheck is a construct that we've deprecated because there's no way it can sanely work - it raises exceptions through the poll loop where they cannot reasonably be handled. We've replaced it with asyncSpawn where it is understood that any "independent" tasks must also catch the exceptions that are raised within so as to be fully independent.
In order to make this work, we also ensure (with raises constraints) that no "naked" callbacks raise - this means that poll can assume that anything it calls doesn't raise which drastically simplifies reasoning about correctness - async functions in particular never (directly) raise: they catch all exceptions transport them on the future - all other callbacks must clean up after themselves.
We've replaced it with asyncSpawn where it is understood that any "independent" tasks must also catch the exceptions that are raised within so as to be fully independent.
I did similar thing and use spawn_async, for same reason. Initiating such independent async call conceptually means spawning a child process (the fact that it's implemented as async is irrelevant), and the spawn conveys the intention better.
I guess it's ok to allow independent task to fail, as long as you explicitly acknowledge that.
Sure, though because we're fairly strict on handling errors to avoid leaks (there's lots in the std lib because of exceptions flying around unchecked), we require that this is indeed an explicit choice - it's well worth the few extra lines of code, and has caught numerous issues in said handlers so far.