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

Event for RECOVERABLE networking errors and subsequent retries #1529

Closed
chrisfillmore opened this issue Aug 7, 2018 · 6 comments
Closed
Assignees
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Milestone

Comments

@chrisfillmore
Copy link
Contributor

We would like to know when a networking error occurs which results in a retry. Our application tracks this information for performance metrics purposes.

To implement such a change, it seems NetworkingEngine would need to accept a PlayerInterface, but I don't know how that would work with alternative use cases as in #1297. Perhaps the PlayerInterface could be optional, and NetworkingEngine could do:

// NetworkingEngine#send_
if (error && error.severity == shaka.util.Error.Severity.RECOVERABLE) {
  if (this.playerInterface) {
    this.playerInterface.onNetworkingRetry_(error); // new method
  }
  // Move to the next URI.
  index = (index + 1) % request.uris.length;
  let shakaError = /** @type {shaka.util.Error} */(error);
  return this.send_(type, request, backoff, index, shakaError);
}

Is this feasible?

@theodab theodab added type: enhancement New feature or request and removed needs triage labels Aug 7, 2018
@theodab theodab self-assigned this Aug 7, 2018
@chrisfillmore
Copy link
Contributor Author

@theodab I don't mind sending in a PR if we agree on a design.

@shaka-bot shaka-bot added this to the Backlog milestone Aug 7, 2018
@theodab
Copy link
Collaborator

theodab commented Aug 7, 2018

@chrisfillmore Your design seems reasonable to me, but I should probably do it. @vaage is currently working on #1297, so it'd be best to do this in-house, to avoid collisions.

@theodab
Copy link
Collaborator

theodab commented Aug 7, 2018

I had a discussion with the team, and we decided that it would probably be best to avoid anything that changes the constructor of NetworkingEngine, since it is also being modified as part of #1051. Instead, I will add a method on the networking engine that adds a listener function. So you'd use it like:

player.getNetworkingEngine().addRetryListener((error) => {
  // Your listener code here.
});

@chrisfillmore
Copy link
Contributor Author

Sounds good, thank you!

@theodab
Copy link
Collaborator

theodab commented Aug 14, 2018

Okay, after another round of code review, this should be out soon.
There was some further discussion, and we decided that it should work as an event listener, using our FakeEventListener code. So you'd use it like:

player.getNetworkingEngine().addEventListener('retry', (event) => {
  let error = event.error;
  // Your listener code here.
});

Sorry we changed our minds so often on this!

@joeyparrish joeyparrish modified the milestones: Backlog, v2.5 Aug 15, 2018
@chrisfillmore
Copy link
Contributor Author

Thanks, @theodab!

@shaka-project shaka-project locked and limited conversation to collaborators Oct 14, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants