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 memory leak #1751

Merged
merged 1 commit into from
Jun 4, 2018
Merged

Conversation

bwallberg
Copy link
Contributor

This PR will...

Fixed AudioStreamController & StreamController not being cleaned up

  • onHandlerDestroying() is overriden by said controllers but doesn't call
    super.onHandlerDestroying() which cases the tick interval to never stop

Why is this Pull Request needed?

Because hls.js is leaking memory after destroy()

Are there any points in the code the reviewer needs to double check?

no

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • no commits have been done in dist folder (we will take care of updating it)
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

- `onHandlerDestroying()` is overriden by said controllers but doesn't call
`super.onHandlerDestroying()` which cases the tick interval to never stop
@tchakabam tchakabam self-requested a review June 4, 2018 15:02
Copy link
Collaborator

@tchakabam tchakabam left a comment

Choose a reason for hiding this comment

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

Great. Thanks so much. This was a stupid bug indeed :)

I would actually like to make using TaskLoop consistent, and this is a good step in the direction, as it is a querk we needed to figure out when overloading those methods. We will have to be careful with this.

Generally, in our roadmap we have an item saying we would like to run one single scheduler for everything actually, centrally manage all the intervals and timeouts, ideally only have on interval running that processes the tasks (which is the idea of task-loop actually, but right now it's more of a thing trying to reduce code redundancy across all the components using some sort of event-loop scheduling via timeout/interval). A next step could be actually having a singleton interval that consumer dispatch tasks on instead of every component having it's own interval loop.

This is a good fix anyway! Thanks!

It would be nice if we find a trick to have components extend from TaskLoop but not have to care about de-initializing it like this. For example we could actually have them not overload these methods but rather have TaskLoop have it's own hooks that are supposed to be overloaded by subclass, and which are being called by the TaskLoop destroy-listeners.

@tchakabam
Copy link
Collaborator

👍 lgtm

Copy link
Collaborator

@johnBartos johnBartos left a comment

Choose a reason for hiding this comment

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

Actually, does this matter? onHandlerDestroyed is not implemented except in the leaf class (i.e. stream-controller).

@johnBartos
Copy link
Collaborator

Maybe I'm missing something ^

Copy link
Collaborator

@johnBartos johnBartos left a comment

Choose a reason for hiding this comment

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

Nevermind, local branch is out of date

@johnBartos johnBartos merged commit a7ca28d into video-dev:master Jun 4, 2018
@tchakabam tchakabam added this to the 0.10.0 milestone Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants