http-kit channel reuse means chsk-send! occasionally responds to the incorrect HTTP request #260
Comments
osbert
changed the title
|
Hi Osbert, thanks a lot for the clear, detailed report - this was very helpful.
It does, but I'll definitely need to dig some more to get a full handle on precisely what's happening + what the possible solutions look like. Just have my hands very full for the next week or two, so it's unlikely I'll be able to look at this right away. Will try make it a priority soon as I can. Some things you or someone else could do so long to assist:
These would save me some significant time, so make it likelier that I can try get to a solution quicker if I have some random free moments. Otherwise any other thoughts/observations very welcome! Cheers :-) |
|
I haven't looked at the other server adapters, but upon further reflection I think this should be considered a bug in http-kit (which I notice you're also the maintainer for so I'm comfortable starting the discussion here). I think a better approach in http-kit would be to create a unique channel on a per-request basis to prevent reuse of the server channel. http-kit has to assume that library users will be holding onto these channels; that you can occasionally have this channel reference and send data back incorrectly seems like a bug. I think treating it as an http-kit bug would also make it much easier to reproduce. Let me know if you agree and I can try to refile for http-kit and provide a suitable test case there. Thanks! |
|
Filing a bug + test with http-kit would be very helpful, thanks Osbert! |
|
Hi Osbert, any update on this? Thanks! |
|
Sorry I've been too busy to look at this further. Will update when I can. |
|
No problem! Appreciate it, thanks :-) |
|
@osbert Hi Osbert, thanks again for the really detailed work on this! Am going to try address this from 2x directions:
|
…of channels (@osbert) It can be dangerous to re-use channels for new HTTP requests. A user might hold on to a channel, and reasonably expect it to *stay closed* after use. By re-using channels, we risk a user unintentionally using a channel held from req1 that's since been RE-OPENED for req2 (e.g. a completely unrelated request). This is more than theoretical- the issue affected Sente, Ref. taoensso/sente#260 Big thanks to @osbert for identifying this risk, and for the proposed test and fix. Commit marked as EXPERIMENTAL since possible performance implications of this change have not yet been measured, and alternative solutions not yet exhaustively considered. Still, since the behaviour prior to this commit is dangerous - it seems reasonable to adopt the present fix as preliminary until further work can be done to either: 1. Establish that this approach has limited or acceptable downsides, or 2. A better alternative can be found
… `conns_` during handshake Before this commit: Server channels (schs) for Ajax GET requests destined for handshaking are unnecessarily added to `conns_` before being closed to issue handshake. Sente presumed that once used, such schs would be permanently closed and all future send attempts against the sch would just fail. But as it turns out, @osbert identified [1][2] that at least http-kit may re-use (and re-open) schs for unrelated future requests. This could lead to cases like the following: - Handshake `req1` comes in, `sch1` gets added to `conns_` then closed. - `sch1` gets re-used (and re-opened) by http-kit for unrelated `req2`. - Before `req2` can respond, a broadcast call is made against `sch1`, which is still in `conns_`. The effect: random broadcast data incorrectly being sent to an unrelated request. After this commit: Server channels (schs) for Ajax GET requests destined for handshaking are never added to `conns_`, preventing the possibility of handshake schs from being held for broadcast after closing. Instead, `conns_` contains only schs intended for long-polling (broadcast), and atomically removed from `conns_` on first use+close. This should prevent the issue described above. Separately, a change [2] is also being introduced upstream to http-kit for server-side prevention of this kind of unintentional re-use of channels. [1] #260 [2] http-kit/http-kit#375
… `conns_` during handshake Before this commit: Server channels (schs) for Ajax GET requests destined for handshaking are unnecessarily added to `conns_` before being closed to issue handshake. Sente presumed that once used, such schs would be permanently closed and all future send attempts against the sch would just fail. But as it turns out, @osbert identified [1][2] that at least http-kit may re-use (and re-open) schs for unrelated future requests. This could lead to cases like the following: - Handshake `req1` comes in, `sch1` gets added to `conns_` then closed. - `sch1` gets re-used (and re-opened) by http-kit for unrelated `req2`. - Before `req2` can respond, a broadcast call is made against `sch1`, which is still in `conns_`. The effect: random broadcast data incorrectly being sent to an unrelated request. After this commit: Server channels (schs) for Ajax GET requests destined for handshaking are never added to `conns_`, preventing the possibility of handshake schs from being held for broadcast after closing. Instead, `conns_` contains only schs intended for long-polling (broadcast), and atomically removed from `conns_` on first use+close. This should prevent the issue described above. Separately, a change [2] is also being introduced upstream to http-kit for server-side prevention of this kind of unintentional re-use of channels. [1] #260 [2] http-kit/http-kit#375
I think this is what was being reported in #188, but let me try to offer a better explanation of what's happening and a couple different ideas for fixing, which I gladly welcome other ideas for.
Note that it is possible that this bug only occurs when using AJAX fallback and http-kit.
Because http-kit server channels are (at least I think) a 1:1 mapping to the underlying socket connection, they are reused as it processes HTTP requests. This means that a race condition is possible where
interfaces/sch-send!attempts to send data down, but the channel is actually currently processing a request other than a sente-initiated AJAX long-poll.From the client perspective the net result is that you issued an AJAX request for XYZ but received the result of a
chsk-send!instead. Note that this is particularly prone to happening during handshake since there is no buffering and essentially closes the current long-poll socket immediately.A full example looks something like this:
chsk-send!to CLIENT-ID, and pulls out a validschfromconns_.on-closeimmediately, butsend-buffered-server-evs>ajax-clients!continues using the old copy. Normally you can rely oninterfaces/sch-send!to fail, but in the meantime ...sch-send!to succeed, but this is in response to "/data". Note that this requires "/data" to take a little while to send a response in order for it to get preempted.I have worked around this bug for now by building custom versions of sente and http-kit that expose the underlying http-kit HttpRequest object and hard-coding that the request URI is for the sente chsk endpoint. I have been able to conclusively prove that sente is calling
sch-send!on a server channel with an associated HttpRequest for a URI other than the chsk endpoint.But ... this does not seem like the best way to handle this. The best approach I've been able to come up with is to add a custom client header to sente long-poll and adding a method to
IServerChanto check for this header in a server-specific way to detect if this is a sente AJAX long poll. Ideally I would prefer to be able to work with a ring request so that the logic could be uniform across all server adapters but I couldn't figure out a way to do that since the channels returned are designed to be opaque objects.Let me know your thoughts and if this bug makes sense! I would be happy to clarify and try to provide logging/a more concrete example, but as you can see it's kind of difficult to illustrate properly.
Thanks for your time and a great library.
The text was updated successfully, but these errors were encountered: