Channel reuse can result in incorrect HTTP response

[osbert]

This is related to taoensso/sente#260 but I have since finally taken the time to properly construct a test case and also am able to explain the issue much better.

When multiple HTTP requests are transmitted over a persistent connection, http-kit re-uses the same underlying channel between requests. This causes issues when there is code that attempts to issue multiple delayed send! calls because the channel gets reset back to open during HTTP request processing for each succeeding request over the same socket.

In particular because of how sente is written, the logic to emulate a websocket using ajax is something like, when you have server events queue them up but if the channel is open, go ahead and send. (It is not really clear to me if this should be regarded as a flaw in how sente attempts to emulate websockets. It could be argued sente should not attempt to call send! on the same channel twice but ... I think there may not be a good way of determining that other than the current open? check).

Please see org.httpkit.server-test/test-channel-reuse-async in https://github.com/osbert/http-kit/blob/channel-reuse-test/test/org/httpkit/server_test.clj#L409 for a test that attempts to illustrate the potential issue.

I think one temporary workaround is to send back Connection: close in headers so that your HTTP consumers do not attempt to use a persistent connection which should prevent channel re-use between requests.

Still thinking about how best to fix, but figured I'd share my work so far. Thanks, and please let me know if you have any thoughts or ideas you'd like to me explore.

[osbert]

One suggested fix is the following patch. The intent is to change it so that a new channel is created for every request. Presumably there are some performance implications but those seemed secondary to correctness. The argument is effectively that even if sente is using the API incorrectly, we should be doing what we can prevent incorrect behavior.

modified   src/java/org/httpkit/server/HttpServer.java
@@ -150,9 +150,9 @@ public class HttpServer implements Runnable {
         try {
             boolean sentContinue = false;
             do {
-                AsyncChannel channel = atta.channel;
                 HttpRequest request = atta.decoder.decode(buffer);
                 if (request != null) {
+										AsyncChannel channel = new AsyncChannel(key, this);
                     channel.reset(request);
                     if (request.isWebSocket) {
                         key.attach(new WsAtta(channel, maxWs));

[ptaoussanis]

Osbert's proposed fix has been merged, will be included in upcoming v2.7 beta.
Thanks again for the awesome work @osbert, it's much appreciated! 🚀