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

fix: Add explicit release() for FakeEventTarget #3950

Merged
merged 1 commit into from
Feb 15, 2022
Merged

fix: Add explicit release() for FakeEventTarget #3950

merged 1 commit into from
Feb 15, 2022

Conversation

joeyparrish
Copy link
Member

@joeyparrish joeyparrish commented Feb 14, 2022

Description

Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens. Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then. The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

Checklist:

  • I have signed the Google CLA https://cla.developers.google.com
  • My code follows the style guidelines of this project
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have verified my change on multiple browsers on different platforms
  • I have run ./build/all.py and the build passes
  • I have run ./build/test.py and all tests pass

Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Feb 14, 2022
Copy link
Contributor

@JulianDomingo JulianDomingo left a comment

Choose a reason for hiding this comment

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

Discussed offline regarding understanding of how Shaka does garbage collection / resource management.

@joeyparrish joeyparrish merged commit f1c1585 into shaka-project:master Feb 15, 2022
@joeyparrish
Copy link
Member Author

Thanks, y'all!

@joeyparrish joeyparrish deleted the release-event-listeners branch February 15, 2022 20:06
joeyparrish added a commit that referenced this pull request Feb 16, 2022
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
joeyparrish added a commit that referenced this pull request Feb 16, 2022
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
joeyparrish added a commit that referenced this pull request Feb 16, 2022
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
joeyparrish added a commit that referenced this pull request Feb 16, 2022
Before, we would count on all event listeners for FakeEventTargets to
be cleaned up by the object that listens.  Now, FakeEventTarget
implements IReleasable, so that all listeners are removed when owners
call release().

For objects extending FakeEventTarget and also implementing
IDestroyable, the destroy() methods will call out to super.release()
to clean up listeners then.  The owner should use destroy() in those
cases.

Issue #3949 (memory leak in DASH live streams with inband EventStream)
@avelad avelad added this to the v4.0 milestone May 4, 2022
@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.
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants