Merged Pull Requests #45 & #46 #48
Conversation
from: terminate(_Req, _Service) -> to: terminate(_Reason, _Req, _Service) ->
…ck in 5 seconds Timeout mechanism is broken with regard to websockets. cowboy process might be overloaded by incoming messages (And session not being able to pick them up quickly enough). If that happens, it's quite likely that session process will receive a timeout after 5 seconds of not having the 'reply' call. This seems to be less of a big deal for non-ws transports as they don't neccesairly come via a signle bottleneck process - receiver is different than sender.
Copy OTP's paramaterized module parse-transform (https://github.com/erlang/pmod_transform) into the source tree; add compiler pragmas to files that require parameterized modules to use the parse-transform (sockjs_pmod_pt). The module implementing the parse-transform has been renamed, as per OTP's suggestion, to avoid name clashes.
|
@dergraf - how can we possibly QA such a massive set of changes? I'd rather handle the two pull requests separately. |
|
@hyperthunk true, but both pr's were sitting there since quite some time and it looked like for some reasons they're not getting merged. maybe nobody has time to merge and test it, so that's what I did. If it's not needed anymore I am happy to close my pr. |
|
Argh, I don't want to be awkward about this, and I do appreciate that these commits have been sitting around forever. I was hoping to merge them today, but I've had too many other things on my plate to go through them properly. Also I've got to consider the impact they'll have on future versions of RabbitMQ, where we use them in our webstomp plugin (and haven't yet upgraded to the latest cowboy). I'll have a discussion about this and see if it's possible to merge them in without causing us any headaches. The tests don't all pass for me either, but I suspect that's something to do with my environment being set up wrong. |
|
new sockjs-erlang tag could also be a solution. but great to know that you are working on that. thanks, let me close this one. |
|
@hyperthunk from what i wrote about my old merge request, all sock-js unit-test run successfully, except those that can not run, because in newer cowboy releases support for older types of websocket connections got removed. I've not tested this new merge request yet. |
|
@gebi - as with your previous pull request, there's so many commits that it'll take me the best part of an afternoon (if not longer) just to review them. Since I'm not a maintainer, I can't commit that kind of time right now and my day job (RabbitMQ) does care about sockjs-erlang, since we use it in some of our plugins, but we have other priorities to attend to first. This work will get done eventually, but as per my comments in pull request #46, if you could size down the patch so that it only deals with cowboy 0.8 support and not a myriad of other things I know nothing about, that would speed things up a lot. Cheers. |
|
@hyperthunk i'll try to get the minimal patches necessary for cowboy 0.8.x support rebased onto upstream/master (is that right?) and resubmit it as a new pulll-request. |
|
That would be fantastic. Thanks! |
The Pull Requests #45 and #46 are both essential to run sockjs-erlang with the current versions of Erlang and Cowboy.
This might save you some work when merging all together.