FIX: Do not disable chunking on cancel/error (#325)

If the first chunk of a message takes more than 3s, we assume an intermediate proxy is buffering data and drop into dont-chunk mode. Previously, this 3s timer would continue even if a request was aborted before the first chunk arrived (e.g. channel subscriptions change in quick succession). This commit ensures the timer is cancelled when the request is aborted.

CHANGELOG

@@ -1,5 +1,12 @@
 FUTURE
 
+
+13-01-2023
+
+- Version 4.2.1
+
+  - FIX: Do not disable chunking on cancel/error
+
 06-01-2023
 
 - Version 4.2.1

assets/message-bus.js

@@ -205,10 +205,11 @@
       return handle_progress(payload, endChunk + separator.length);
     };
 
+    var chunkedTimeout;
     var disableChunked = function () {
       if (me.longPoll) {
         me.longPoll.abort();
-        chunkedBackoff = 30;
+        chunkedBackoff = me.retryChunkedAfterRequests;
       }
     };
 
@@ -235,9 +236,9 @@
         chunked: chunked,
         onProgressListener: function (xhr) {
           var position = 0;
-          // if it takes longer than 3000 ms to get first chunk, we have some proxy
-          // this is messing with us, so just backoff from using chunked for now
-          var chunkedTimeout = setTimeout(disableChunked, 3000);
+          // if it takes longer than firstChunkTimeout to get first chunk, there may be a proxy
+          // buffering the response. Switch to non-chunked long-polling mode.
+          chunkedTimeout = setTimeout(disableChunked, me.firstChunkTimeout);
           return (xhr.onprogress = function () {
             clearTimeout(chunkedTimeout);
             if (
@@ -269,6 +270,7 @@
         }
       },
       error: function (xhr, textStatus) {
+        clearTimeout(chunkedTimeout);
         if (xhr.status === 429) {
           var tryAfter =
             parseInt(
@@ -364,6 +366,8 @@
     clientId: clientId,
     alwaysLongPoll: false,
     shouldLongPollCallback: undefined,
+    firstChunkTimeout: 3000,
+    retryChunkedAfterRequests: 30,
     baseUrl: baseUrl,
     headers: {},
     ajax: typeof jQuery !== "undefined" && jQuery.ajax,

lib/message_bus/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module MessageBus
-  VERSION = "4.3.1"
+  VERSION = "4.3.2"
 end

spec/assets/SpecHelper.js

@@ -34,17 +34,32 @@ beforeEach(function () {
     this.readyState = 4
     this.responseText = encodeChunks(this, spec.responseChunks);
     this.status = spec.responseStatus;
-    if (this.onprogress){ this.onprogress(); }
-    this.onreadystatechange()
+
+    spec.requestStarted?.();
+
+    const complete = () => {
+      if(this.statusText === "abort"){
+        return;
+      }
+      this.onprogress?.();
+      this.onreadystatechange();
+    }
+
+    if(spec.delayResponsePromise){
+      spec.delayResponsePromise.then(() => complete())
+      spec.delayResponsePromise = null;
+    }else{
+      complete();
+    }
   }
 
   MockedXMLHttpRequest.prototype.open              = function(){ }
 
   MockedXMLHttpRequest.prototype.abort             = function(){
     this.readyState = 4
     this.responseText = '';
-    this.statusText = '';
-    this.status = 400;
+    this.statusText = 'abort';
+    this.status = 0;
     this.onreadystatechange()
   }
 
@@ -68,14 +83,17 @@ beforeEach(function () {
     "Content-Type": 'text/plain; charset=utf-8',
   };
 
+  MessageBus.enableChunkedEncoding = true;
+  MessageBus.firstChunkTimeout = 3000;
+  MessageBus.retryChunkedAfterRequests = 1;
+
   MessageBus.start();
 });
 
 afterEach(function(){
   MessageBus.stop()
   MessageBus.callbacks.splice(0, MessageBus.callbacks.length)
   MessageBus.shouldLongPollCallback = null;
-  MessageBus.enableChunkedEncoding = true;
 });
 
 window.testMB = function(description, testFn, path, data){

spec/assets/message-bus.spec.js

@@ -166,4 +166,63 @@ describe("Messagebus", function () {
     const nextPollScheduledIn = window.setTimeout.calls.mostRecent().args[1];
     expect(nextPollScheduledIn).toEqual(MessageBus.minPollInterval);
   });
+
+  it("enters don't-chunk-mode if first chunk times out", async function () {
+    spyOn(this.MockedXMLHttpRequest.prototype, "send").and.callThrough();
+    spyOn(
+      this.MockedXMLHttpRequest.prototype,
+      "setRequestHeader"
+    ).and.callThrough();
+
+    let resolveFirstResponse;
+    this.delayResponsePromise = new Promise(
+      (resolve) => (resolveFirstResponse = resolve)
+    );
+    MessageBus.firstChunkTimeout = 50;
+
+    await new Promise((resolve) => MessageBus.subscribe("/test", resolve));
+    resolveFirstResponse();
+
+    const calls =
+      this.MockedXMLHttpRequest.prototype.setRequestHeader.calls.all();
+
+    const dontChunkCalls = calls.filter((c) => c.args[0] === "Dont-Chunk");
+    expect(dontChunkCalls.length).toEqual(1);
+  });
+
+  it("doesn't enter don't-chunk-mode if aborted before first chunk", async function () {
+    spyOn(
+      this.MockedXMLHttpRequest.prototype,
+      "setRequestHeader"
+    ).and.callThrough();
+
+    this.delayResponsePromise = new Promise(() => {});
+    MessageBus.firstChunkTimeout = 300;
+
+    const requestWasStarted = new Promise(
+      (resolve) => (this.requestStarted = resolve)
+    );
+
+    // Trigger request
+    const subscribedPromise = new Promise((resolve) =>
+      MessageBus.subscribe("/test", resolve)
+    );
+
+    await requestWasStarted;
+
+    // Change subscription (triggers an abort and re-poll)
+    MessageBus.subscribe("/test2", () => {});
+
+    // Wait for stuff to settle
+    await subscribedPromise;
+
+    // Wait 300ms to ensure dontChunk timeout has passed
+    await new Promise((resolve) => setTimeout(resolve, 300));
+
+    const calls =
+      this.MockedXMLHttpRequest.prototype.setRequestHeader.calls.all();
+
+    const dontChunkCalls = calls.filter((c) => c.args[0] === "Dont-Chunk");
+    expect(dontChunkCalls.length).toEqual(0);
+  });
 });