Continuing the journey of webby things based on chronos, we now have a first version out of nim-ws.
nim-ws, very imaginatively named, is a library for websocket communication - we're currently using it to communicate with services that offer an API over websockets as a slightly more efficient alternative to regular http requests - in particular, Ethereum can be accessed this way, often via nim-json-rpc which recently gained nim-ws support.
There's a lot of good things to say about the library, but what's nice about it in particular is that it passes the entire autobahn test suite, complete with compression, extension support and all!
Next up, we'll be adding support for websocket endpoints in nim-libp2p to enable better communication options for browser-based applications, in particular our suite of privacy-preserving protocols found in nim-waku.
Have fun with it :)
Nice beginning for a socks library!
This a a problem often encountered with asyncdispatch and the Nim std library itself, but if you want to use it for production projects, I'd recommend you audit the code for leaks and error handling in general - code like https://github.com/enthus1ast/nimSocks/blob/19defc5fe3e131efdd2f4544dbb71323cb1bf853/nimSocks/server.nim#L159 for example is not exception safe and is prone to cause the server to leak file descriptors - this is tied to the use of asyncCheck which in itself is broken in this regard and should probably be avoided.
Can you elaborate on how that code would result in fd leaks?
By not handling exceptions, which in turn means close is not called on the socket.
Also, using asyncCheck causes the exception that send raises to pop out of the poll call instead at which point the context for the exception is lost, because you don't really know which async task the exception originated from - this leads to a number of issues which is why we've deprectated it in chronos.
Technically one could add finalizers to AsyncSocket and friends but due to the non-deterministic behavior of the GC, this is still not a solution - it would be vulnerable to exhaustion attacks and a number of other issues which makes the whole construct non-suitable for production use.
I don't think that specific line of code you linked is problematic, consider the same code with synchronous sockets: the exception would bubble up until a point where it's caught where you'd have access to the socket and would then close it (or preferably have a defer set up to close it). If you were running the function in a new thread then you'd be expected to handle the resource clean up yourself. If there is an unhandled exception in a thread then your whole application will fail.
This is the same with async, when you call asyncCheck you're asking for your process to crash when the async proc you run throws an exception. All asyncCheck does is set a callback to do that. Ideally it shouldn't be necessary to have this asyncCheck written all over the place, but when I implemented async/await it wasn't trivial to do this implicitly.
I don't think that specific line of code you linked is problematic
I highlighted the line as an example of one of many lines in that code where exceptions are raised, but are not caught. Not catching them leads to leaks and/or crashes because of missing exception handlers / defer / whatever your preferred handling mechanism is. If that is not problematic, I'm not sure what is.
when you call asyncCheck you're asking for your process to crash
No, you ask that the exception is transferred to the function calling runForever where it can be caught - this is a design issue with asyncCheck which leads to poor error handling at best, and like you say, crashes at worst. Whether that's fine or not of course depends on your use case, I guess.
I highlighted the line as an example of one of many lines in that code where exceptions are raised, but are not caught. Not catching them leads to leaks and/or crashes because of missing exception handlers / defer / whatever your preferred handling mechanism is. If that is not problematic, I'm not sure what is.
By that definition pretty much all lines are problematic since most of them can throw. Nim isn't Go and for a good reason, writing error propagation logic manually is a waste of time.
No, you ask that the exception is transferred to the function calling runForever where it can be caught - this is a design issue with asyncCheck which leads to poor error handling at best, and like you say, crashes at worst.
No, the whole point of asyncCheck is to ensure exceptions are not silently ignored and to kill your application if your code's exceptions aren't handled. This mechanism is not there so that you can catch the exceptions in poll. If you are catching these exceptions by wrapping runForever (or poll) with a try...`catch` then you're simply asking for trouble.
I suppose I should have emulated a Rust panic and just called quit instead of raising. But really, you're making this out to be far worse than it is. Idiomatic async code always has a poll/runForever/waitFor at the top-level and it never checks for exceptions at the poll call, so your app will crash and that's by design.
If you don't want it to crash then just define an asyncLog which logs the errors instead. Really this is something that should be defined for you in the stdlib, but since Nim's threading provides a way to override the default exception behaviour for threads maybe we should simply emulate this for async too to change what asyncCheck does.
By that definition pretty much all lines are problematic since most of them can throw.
Lol, try reading the whole sentence, and you might catch its meaning.
No, the whole point of
I hope you'll agree that it's a bit hard to understand what "the whole point" is and when we're asking for trouble, when the function is implemented and works in a different way than what you're describing?
If you don't want it to crash
I'm not sure I follow, are you proposing that defining an asyncLog in my application will resolve the highlighted issue in the socks code or the multitude of similar leaks we find in the std library? I believe this is where the discussion started at least.
Let me reply on the side for
By that definition pretty much all lines are problematic since most of them can throw. Nim isn't Go and for a good reason, writing error propagation logic manually is a waste of time.
I kind of disagree with that. Sure, in Go you have to write the error logic by yourself, but that's not difficult and it does not make the code so terrible to read. it gives you two benefits:
If you don't want it to crash then just define an asyncLog which logs the errors instead. Really this is something that should be defined for you in the stdlib, but since Nim's threading provides a way to override the default exception behaviour for threads maybe we should simply emulate this for async too to change what asyncCheck does.
What's the difference between the asyncLog you suggest and looping around runForever while logging errors? It seems pretty equivalent to me.
In any case, an exception that goes down back to the async runner seems a bug to me. it might be useful to catch it to prevent the whole application from crashing, but it should be avoided and corrected if you see the error.
you can think about the errors and in some cases you might do extra cleanup in case of it you would surely have forgotten to write if you didn't set yourself to write the error handler
The price is very high and it depends on your requirements. In the Nim compiler every call can raise an exception and checking after every call would be ridiculously verbose. Instead of exceptions all I could use instead is in fact quit and then the compiler becomes harder to use as a library...
it makes for beautiful error messages. perhaps a bit verbose, but when you have an error, you know exactly what happened.
Except when you need a stack trace, that is...
There's conflict of interests. Errors are exceptional cases in simple or one-timer run programs and could be ignored. Errors are not exceptional cases for long running programs and could not be ignored.
There are no way to satisfy both use cases (actually, Erlang did it by assembling program as independent isolated chunks, but Erlang approach is not possible for languages with shared memory).
I think simple cases are very important and should be kept simple, and complex cases possible. And, Nim (and Java, Python, Ruby etc.) went that way. Go decided differently, to make simple cases harder but complex cases easier, but that path is not for everyone.
I think {.raises: [].} at the top of the module is a reasonable way.
From my personal experience, this problem is magnified by async. Async itself is harder, but in simple language like JavaScript when everything is made specifically for async, it's more or less ok. But when you try to combine async + Nim (a very complicated language with many different concepts) + immature platform. It's like 3x more complicated and became unmanageable.
... Sure, in Go you have to write the error logic by yourself, but that's not difficult ...
It is difficult and annyoing. In cases when you don't need it.
Go decided differently, to make simple cases harder but complex cases easier, but that path is not for everyone.
I know of no studies on that topic. Mechanically adding if (err) { return err; } and pretending that it means "you always keep thinking about the error case" is a most naive assumption and needs to be backed up by scientific studies.
In the meantime, in C# land, the exception handling mechanism is so effective that even stack overflows or out-of-memory conditions can be effectively handled and the server can keep running.