Prevent base stream from logging after stop #4564
Conversation
…tting fragment loading following a rejection
|
Screenshot of Sentry event that identified issue:
|
| @@ -377,6 +377,9 @@ export default class BaseStreamController | |||
| this._handleFragmentLoadComplete(data); | |||
| }) | |||
| .catch((reason) => { | |||
| if (this.state === State.STOPPED) { | |||
There was a problem hiding this comment.
It isn't exactly that it's stopped that it's an issue, but more so that destroy has been called.
Adding an existence check here, but also if you could set this.log, and this.warn as possibly null, TypeScript should warn of all the locations that it needs to be an optional call. You could fix by changing it to this.warn?.(reason) as a shorthand.
There was a problem hiding this comment.
Thanks for catching and submitting the PR!
There was a problem hiding this comment.
@itsjamie - You're welcome for the PR. Do you recommend that I make these changes or would you prefer to use my original solution?
There was a problem hiding this comment.
We can merge the PR as it exists now because this.resetFragmentLoading wouldn't have been running prior so likely should run post merge unless it should be in the case when the player has been destroyed.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Ran local CI on macOS Chrome 101, 129 passing. Merging down. |
Prevent base stream from logging after stop
This PR will...
This PR will prevent the base stream controller from attempting to log after the state has changed to STOPPED.
Why is this Pull Request needed?
The logger is set to null when the state is STOPPED; this pull request prevents an unhandled type error.
Are there any points in the code the reviewer needs to double check?
Should the code check if
this.warnis a function instead of checking the state?Resolves issues:
#4092
Checklist