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.

Sign up for GitHub

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

Jump to bottom

Store unload promise on player #613

Closed
wants to merge 3 commits into from
Closed

Store unload promise on player #613

wants to merge 3 commits into from

Conversation

chrisfillmore
Copy link
Contributor

Fix for #612

Chris Fillmore added 3 commits November 29, 2016 14:20
Add Promise property to Player to store unloading process, if one
d20393a 
is ongoing.

Add check in Player.prototype.unload to see if an unload is
currently underway, and if so, to return that Promise instead of
creating a new one.

Add method for creating unloading process Promise which nulls
itself out when unloading is complete.
Merge branch 'master' into bugFix/addUnloadPromiseOnPlayer
790c7a5
Updated CONTRIBUTORS
57d5cf5
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@chrisfillmore chrisfillmore mentioned this pull request Nov 30, 2016
Closed
@chrisfillmore
Copy link
Contributor Author

I signed the CLA!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@joeyparrish joeyparrish self-assigned this Nov 30, 2016
@joeyparrish
Copy link
Member

joeyparrish commented Nov 30, 2016 •
edited
Loading

Hi Chris,

Thanks for the PR! I'd like to see an additional test for this case in test/player_unit.js. We already have several test cases for various orderings of load and unload in the section that starts with describe('load/unload', ..., so that would be a good place to add a new test.

You can run the tests locally with python build/test.py. Please make sure that the new test fails without your fix.

Thanks again for contributing!

joeyparrish
joeyparrish reviewed Nov 30, 2016
View reviewed changes
lib/player.js
* the unload promise.
* @return {!Promise}
*/
shaka.Player.prototype.createUnloadPromise_ = function(unloadProcess) {
Copy link
Member

Choose a reason for hiding this comment

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

Since you're passing in an unresolved Promise chain, and to keep terminology consistent with other parts of this file, change "unloadProcess" to "unloadChain" or "unloadPromiseChain".

@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Nov 30, 2016
@chrisfillmore
Copy link
Contributor Author

Hi Joey, looks like I am probably using my work address for git but my personal address for the CLA, so I'll have to resolve that in the morning. Thanks.

@chrisfillmore
Copy link
Contributor Author

I added my work email to my Google email accounts, help me @googlebot you're my only hope

@googlebot
Copy link

CLAs look good, thanks!

shaka-bot pushed a commit that referenced this pull request Dec 21, 2016
@TheModMaker
Fix unload/load bug in Player.
798b1e0 
Based on PR #613

There was a bug where calling unload() right before calling load()
would cause a race that would sometimes cause a failure.  This is
because the fields are reset before the unload is complete so the
second call to unload() (from inside load) will complete immediately.

So now we store the unload Promise chain while it is in progress so
we can wait on it from load().

Closes #612

Change-Id: I6c0cdd931827d709fc41322edd51fe10e4aa87ae
@TheModMaker
Copy link
Contributor

Thanks for the contribution, but we decided to fix this slightly differently.

joeyparrish pushed a commit that referenced this pull request Jan 7, 2017
@TheModMaker @joeyparrish
Fix unload/load bug in Player.
18ed1af 
Based on PR #613

There was a bug where calling unload() right before calling load()
would cause a race that would sometimes cause a failure.  This is
because the fields are reset before the unload is complete so the
second call to unload() (from inside load) will complete immediately.

So now we store the unload Promise chain while it is in progress so
we can wait on it from load().

Closes #612

Change-Id: I6c0cdd931827d709fc41322edd51fe10e4aa87ae
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish left review comments

Assignees

@joeyparrish joeyparrish

Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chrisfillmore @googlebot @joeyparrish @TheModMaker