Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't wait for requests to finish when encountering an error in media… #286

Merged
merged 1 commit into from
Nov 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 20 additions & 19 deletions src/media-segment-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Does this change mean that we might not get the most "pertinent" error on the player?
  • Or does it actually end up returning the most relevant error by bubbling up the first error?
  • Is there a case where we would want other requests in the group to be made even if one failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, before this change I don't think it was possible for there to be more than just aborted/the original error. So this should have no change.

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
Expand All @@ -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
Copy link
Contributor

@ldayananda ldayananda Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😬

//
// 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).
Copy link
Contributor

@ldayananda ldayananda Nov 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😬

// 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);
}
Expand Down
44 changes: 44 additions & 0 deletions test/media-segment-request.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to rename abortedKeyReq so it's clear that we're overriding default behavior

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, can we mimic this scenario by setting the XHR state to unsent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried that, and we can, the only thing I was wary of after I wrote it up is I still wanted to have a way to verify that we called abort on the key request, otherwise, since we mock other aspects of the XHR, I'd worry that the state would always be in unsent for some reason and we didn't actually call abort properly. Though maybe that would cause problems elsewhere. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense for the purpose of verifying we called abort. Can we also verify that the XHR state changed in the test or does the mocking make that difficult/not useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XHR state for the key request optimally shouldn't actually change if we want to reflect the browser behavior (that was the cause of the bug, that it remains in its original, unsent, state), but with the mocking and setup, the state does change (why we override the abort in this case).

};
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();
Expand Down