This looks pretty interesting, great work!
Out of curiosity, why did you roll your own special process, instead of using e.g. GenServer?
sasajuric
Author of Elixir In Action
Azolo
It’s because I’m too curious for my own good.
When I realized, “Oh right, this should play nice with OTP.” I decided to learn what made a process “special” instead of just putting it in a GenServer.
There was a point where I was frustrated and started to roll it into a GenServer, but at that point I also realized the design choice that was frustrating me and changed it.
1
idi527
If it’s not much of a bother, could you briefly explain how it is different from websocket_client [0]?
[0] GitHub - sanmiguel/websocket_client: Erlang websocket client (ws and wss supported) · GitHub
1
Azolo
Sure, but first let me say there are two different websocket_client repos that I used.
The first one was the one listed on hex.pm(which is the one you linked). It implements gen_fsm and it more actively maintained. However, there are also a couple bugs. Most of them were already reported issues, but one in particular was that terminate was being called consistently in one of my cases. However it was fickle and I had problems reproducing it, but it was causing my tests to fail 1/15 times. So when I tried to jump and trace it, I was overwhelmed by the sheer number of things the repo was trying to do.
So I looked at the original fork. It didn’t have any of the bugs I experienced but also wasn’t an OTP special process and didn’t wait for the server to close the socket (as per the spec). I think I also had some problems with close frames that contained a payload. But this repo is no longer maintained in favor of the one above.
So, I decided to to write my own. I wanted it to be some where in between. Being compatible with OTP, but simple enough that someone could jump into and be able to submit simple bug-free code without being overwhelmed.
Also, WebSockex is has great test coverage. 
2
michalmuskala
The remark that it implements special process by hand made me interested, so I had a look at the code. Here are some things I spotted, I hope you don’t mind.
- you don’t handle the regular naming conventions - it would be nice to be able to pass the
nameoption like with all the Elixir’s stdlib behaviours. When you do this, there are some race conditions to consider, so I would advise looking at using the:genmodule that powers all the Erlang behaviours (it’s not documented publicly, but I saw many projects using it, I’m not sure how bad of an idea that is - but it definitely handles a lot of common scenarios). - whenever you call user module callbacks, they should be wrapped in a
tryand errors should be handled appropriately (e.g. you want to call the terminate callback in that case before dying). - it’s nice to use
:sys.handle_debugfor any messages received and sent out (or any “major” events that happen in a process) - this is really useful when things go wrong. - in the receive loop, you probably want to look for the
{:EXIT, parent, reason}message in case the user starts trapping exits in the process. The special process has to exit with the same reason as the parent in case that happens. - it looks like you don’t implement the
system_code_change/4callback - there’s an additional
format_status/2callback you can implement to control what information is returned on:sys.get_statusand:sys.get_stateused, for example, by observer.
5
sasajuric
Author of Elixir In Action
I absolutely appreciate and encourage trying different things, and even overcomplicating the code for the purpose of learning and gaining a deeper understanding. That being said, if learning is the only motivation for using your own special process here, and assuming that this library is meant to be used by others and contributed to, I think it might be wise to switch to GenServer. Doing this would make it easier to solve (and in some cases automatically solve) issues mentioned by @michalmuskala.
It might also simplify some future additions. For example, I think that calls should be supported as well, and this would be pretty simple if the process was a GenServer. Making the interface of your behaviour more similar to GenServer by supporting timeouts and hibernate options in return tuples, as well as stop option would also be nice. Again, this should be fairly trivial if your behaviour was powered by GenServer.
I have similar sentiments about the current state of WS clients. About a year ago, I looked into a couple of them, and wasn’t really completely pleased with any. In the end, I settled for this branch. I like that your project starts with a clean slate, and has good test coverage. That seems very promising!
3
Azolo
Awesome feedback, thanks a ton. A couple of other things I didn’t mention or am starting to remember now.
So the :gen module implements the call behavior. Which I had a really long thought process where I didn’t really want to have any synchronous calls because of the behavior of WebSockets and the way reconnects are implemented. That may change in the future but for now I don’t want any thing that can Timeout.
But I don’t mind adding a :name option. That actually doesn’t look too hard.
In the :gen module it looks like it checks during start and then again during the init function when it tries to register the process.
Ahh, good point. I did that for some of them, but I don’t think that it’s tested and it isn’t consistent.
Yeah, that is me being lazy. I also need to add an error message on exit.
I totally didn’t know that. Thanks.
Oops. Thanks, incidentally I have no idea how to test this.
This is me being lazy again. 
Thanks again for the advice!
0
Azolo
This made me remember why I didn’t use GenServer. I didn’t want those behaviors to be available because it could mess the handling of the control frames and I didn’t want to move the WebSocket into another child process.
I had the idea of writing another module based off GenServer that would wrap a WebSockex connection. This would make doing complex things like synchronous sends possible without messing too much with the flow that the WebSocket spec asks for.
0
michalmuskala
The fact that call is synchronous for the caller, does not mean it’s synchronous for the server. Using an explicit GenServer.reply/1 you can reply to the message before returning from the handle_call function, after or even from a completely different process.
0
sasajuric
Author of Elixir In Action
Just because your behaviour is internally powered by GenServer doesn’t mean it has to expose everything GenServer does. So for example, if the timeout option doesn’t make sense in WebSockex, you don’t need to support it in your own behaviour.
0