Minor websockets improvements
This MR makes small improvements to the websocket-related part of the codebase, mostly as a stepping stone for #392 (closed) (cc @anoe).
@cgenie alas I'm out of hours for the whole week and by the sound of it I will have to slow down things for a bit, but as a general commentary (don't take this personally, of course :) ) the current websocket implementation has a few problems -- I understand this is mostly a PoC , but I hope this commentary could be nevertheless useful to you:
- The fact that we are using nanomsg leaks in a few places (i.e. check the explicit imports for nanomsg) -- if would be better if these were segregated in some "transport-layer-specific" modules, so that ripping it apart in favour of
nng
or0mq
is easier in the future; - In the similar vein, things like the
Dispatcher
(see one of my commits) were too "open" -- by making them opaque and having specialised accessors liketerminatedDispatcher
means we can change the internal implementation without leaking the knowledge to the client functions. Previously the client would have to callkillThread
to terminate aDispatcher
, leaking the internal fact we were usingforkIO
, and forcing us (if we were to switch to, say,async
) to amend all the call sites of that functions to remove thekillThread
, which is not ideal; - Directly related to
2.
, most if not all the code I have seen lacks support for handling synchronous and asynchronous exceptions -- in other terms, we don't do any kind of resource cleanup, and we can leave open sockets / connections to nanomsg etc. I recommend reading this blogpost for an example of what I'm talking about; - Related to
2.
and3.
, we are using too manyforkIO
in the code, which is, again, fragile. If you read the documentation for forkIO you will see that if a thread spawned viaforkIO
dies, the parent is not notified, and thus our dispatcher or CE might silently die and the entire WS machinery would stall/deadlock -- I recommend usingAsync
and perhaps for now using thelink
function to bind the asyncs to the main thread, so that if any of them dies, the main server dies. This might be drastic and we might want to change it in the future, but for now as we are in the prototyping stage it will tell us if things are going wrong in the WS code, so we can fix it. Then, once things are stable, we can reason about ways to restart the WS machinery if things go wrong, without killing the server, but let's make it correct first. - I have removed
recvMalloc
, see the commit comment to see why. I have opened a few PR against the main repo and asked my colleague Ben Gamari to review/merge them. In general is better to try not have forks, and we should get into the habit of trying to contact the original authors, or at least open an issue rather than forking immediately (and I'm guilty of this myself, we are all in a hurry, after all :) ).
I hope this helps, it's not an exhaustive list, but at this stage it doesn't feel it has to be. Thanks for your work!