From 01f00168962801415739436343db4d404cf445ae Mon Sep 17 00:00:00 2001
From: Garrett Singer <gesinger@gmail.com>
Date: Wed, 7 Nov 2018 12:51:09 -0500
Subject: [PATCH] Don't wait for requests to finish when encountering an error
 in media-segment-request

---
 src/media-segment-request.js       | 39 +++++++++++++-------------
 test/media-segment-request.test.js | 44 ++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/src/media-segment-request.js b/src/media-segment-request.js
index a3d0a6b34..5ce2deb0b 100644
--- a/src/media-segment-request.js
+++ b/src/media-segment-request.js
@@ -298,18 +298,6 @@ const decryptSegment = (decrypter, segment, doneFn) => {
   ]);
 };
 
-/**
- * The purpose of this function is to get the most pertinent error from the
- * array of errors.
- * For instance if a timeout and two aborts occur, then the aborts were
- * likely triggered by the timeout so return that error object.
- */
-const getMostImportantError = (errors) => {
-  return errors.reduce((prev, err) => {
-    return err.code > prev.code ? err : prev;
-  });
-};
-
 /**
  * This function waits for all XHRs to finish (with either success or failure)
  * before continueing processing via it's callback. The function gathers errors
@@ -322,26 +310,39 @@ const getMostImportantError = (errors) => {
  *                            downloaded and any decryption completed
  */
 const waitForCompletion = (activeXhrs, decrypter, doneFn) => {
-  let errors = [];
   let count = 0;
+  let didError = false;
 
   return (error, segment) => {
+    if (didError) {
+      return;
+    }
+
     if (error) {
+      didError = true;
       // If there are errors, we have to abort any outstanding requests
       abortAll(activeXhrs);
-      errors.push(error);
+
+      // Even though the requests above are aborted, and in theory we could wait until we
+      // handle the aborted events from those requests, there are some cases where we may
+      // never get an aborted event. For instance, if the network connection is lost and
+      // there were two requests, the first may have triggered an error immediately, while
+      // the second request remains unsent. In that case, the aborted algorithm will not
+      // trigger an abort: see https://xhr.spec.whatwg.org/#the-abort()-method
+      //
+      // We also can't rely on the ready state of the XHR, since the request that
+      // triggered the connection error may also show as a ready state of 0 (unsent).
+      // Therefore, we have to finish this group of requests immediately after the first
+      // seen error.
+      return doneFn(error, segment);
     }
+
     count += 1;
 
     if (count === activeXhrs.length) {
       // Keep track of when *all* of the requests have completed
       segment.endOfAllRequests = Date.now();
 
-      if (errors.length > 0) {
-        const worstError = getMostImportantError(errors);
-
-        return doneFn(worstError, segment);
-      }
       if (segment.encryptedBytes) {
         return decryptSegment(decrypter, segment, doneFn);
       }
diff --git a/test/media-segment-request.test.js b/test/media-segment-request.test.js
index 766ec46be..e0042a607 100644
--- a/test/media-segment-request.test.js
+++ b/test/media-segment-request.test.js
@@ -193,6 +193,50 @@ QUnit.test('cancels outstanding key requests on timeout', function(assert) {
   this.clock.tick(2000);
 });
 
+QUnit.test('does not wait for other requests to finish when one request errors',
+function(assert) {
+  let keyReq;
+  let abortedKeyReq = false;
+  const done = assert.async();
+
+  assert.expect(8);
+  mediaSegmentRequest(
+    this.xhr,
+    this.xhrOptions,
+    this.noop,
+    this.noop,
+    {
+      resolvedUri: '0-test.ts',
+      key: {
+        resolvedUri: '0-key.php'
+      }
+    },
+    this.noop,
+    (error, segmentData) => {
+      assert.notOk(keyReq.aborted, 'did not run original abort function');
+      assert.ok(abortedKeyReq, 'ran overridden abort function');
+      assert.equal(error.code, REQUEST_ERRORS.FAILURE, 'request failed');
+
+      done();
+    });
+  assert.equal(this.requests.length, 2, 'there are two requests');
+
+  keyReq = this.requests.shift();
+  // Typically, an abort will run the error algorithm for an XHR, however, in certain
+  // cases (e.g., if the request is unsent), the error algorithm will not be run and
+  // the request will never "finish." In order to mimic this behavior, override the
+  // default abort function so that it doesn't finish.
+  keyReq.abort = () => {
+    abortedKeyReq = true;
+  };
+  const segmentReq = this.requests.shift();
+
+  assert.equal(keyReq.uri, '0-key.php', 'the first request is for a key');
+  assert.equal(segmentReq.uri, '0-test.ts', 'the second request is for a segment');
+
+  segmentReq.respond(500, null, '');
+});
+
 QUnit.test('the key response is converted to the correct format', function(assert) {
   let keyReq;
   const done = assert.async();