Usage parity of HTTP_ERROR in networking plugins

[chrisfillmore]

I believe the usage of 1002 HTTP_ERROR is not exactly comparable between HttpFetchPlugin and HttpXhrPlugin. Specifically, I believe this check in HttpFetchPlugin is not correct:

} else if (error.severity == undefined) {
  // Replace non-shaka errors with a generic HTTP_ERROR.
  return Promise.reject(new shaka.util.Error(
      shaka.util.Error.Severity.RECOVERABLE,
      shaka.util.Error.Category.NETWORK,
      shaka.util.Error.Code.HTTP_ERROR,
      uri, error, requestType));

If HttpPluginUtils.makeResponse (used earlier in the promise chain) threw a ReferenceError due to a typo during development, it would get caught and wrapped in HTTP_ERROR. The same could not happen in HttpXhrPlugin. I think it is preferable for such an error to escalate immediately and infect your calling code, so that it is caught easily.

Consider this change, to catch fetch promise rejection immediately:

let promise = fetch(uri, init).catch((error) => {
  return Promise.reject(new shaka.util.Error(
      shaka.util.Error.Severity.RECOVERABLE,
      shaka.util.Error.Category.NETWORK,
      shaka.util.Error.Code.HTTP_ERROR,
      uri, error, requestType));
});

promise = promise.then(function(response) {

...

    return shaka.net.HttpPluginUtils.makeResponse(headers,
          arrayBuffer, response.status, uri, response.url, requestType);
    });
  }).catch(function(error) {
    goog.asserts.assert(error instanceof shaka.util.Error, 'Wrong error type!');

You would also be able to assert that the error caught at the end of the chain is a shaka.util.Error, and there would be no fuzzy check for error.severity.

Do you agree?

[TheModMaker]

Sure, that sounds reasonable to me. Note that I would also have that for the arrayBuffer() call to since that could fail. It would also be much easier using async/await and a try/catch. Do you want to send in a PR?

[chrisfillmore]

@TheModMaker since networking plugins synchronously return an AbortableOperation, it seems it would not be possible to use async/await, since I need an explicit reference to the Promise to pass to AbortableOperation.

[TheModMaker]

What you could do is have a separate method that is async and uses await; then in the main plugin call that method and use the returned Promise. But that would be a larger refactor, so you don't have to.

shaka.net.HttpFetchPlugin = function(uri, request, requestType) {
  let controller = new shaka.net.HttpFetchController.AbortController_();
  return new shaka.util.AbortableOperation(
      shaka.net.HttpFetchPlugin.impl_(uri, request, requestType, controller),
      () => { controller.abort(); return Promise.resolve(); });
};

shaka.net.HttpFetchPlugin.impl_ = async function(uri, request, requestType, controller) {
  ...
  try {
    await fetch(...);
  } catch (e) {
    ...
  }
};

[chrisfillmore]

Thanks for the tip, I will see what I can do.