-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Potential bug/leak: middleware cache grows even if no middleware created #8653
Open
BrainCrumbz opened this issue
Mar 22, 2024
· 2 comments
· May be fixed by kerasus/soalaa#1707, kerasus/soalaa#1708, kerasus/soalaa#1711, go-benchmark1/filebrowser#2 or kerasus/soalaa#1715
Open
Potential bug/leak: middleware cache grows even if no middleware created #8653
BrainCrumbz opened this issue
Mar 22, 2024
· 2 comments
· May be fixed by kerasus/soalaa#1707, kerasus/soalaa#1708, kerasus/soalaa#1711, go-benchmark1/filebrowser#2 or kerasus/soalaa#1715
Labels
needs: triage
This issue needs to be reviewed
Comments
A PR would be welcome, thanks. |
7 tasks
PR can be found at #8674 Waiting for comments etc. Bye! |
mister-ben
added a commit
that referenced
this issue
Jul 6, 2024
## Description See issue #8653 ## Specific Changes proposed When in `middleware.js` the function `clearCacheForPlayer` runs, before setting a value to null in middlware caches, it checks if the key exists in the first place. ## Requirements Checklist - [x] Feature implemented / Bug fixed - [x] If necessary, more likely in a feature request than a bug fix - [x] Change has been verified in an actual browser (Chrome, Firefox, IE) - [ ] Unit Tests updated or fixed - [ ] Docs/guides updated - [x] Example created ([starter template on JSBin](https://codepen.io/gkatsev/pen/GwZegv?editors=1000#0)) - [ ] Reviewed by Two Core Contributors --------- Co-authored-by: Giuseppe Piscopo <g.piscopo@braincrumbz.com> Co-authored-by: mister-ben <git@misterben.me>
This was referenced Aug 1, 2024
This was referenced Sep 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Description
TL;DR: when videojs players are instantiated and disposed repeatedly on page,
middlewareInstances
keeps growing with new keys and null values.We are hunting potential memory leaks in our video-based app. Comparing heap snapshots on Windows Chrome and Android Chrome, we noticed a (small?) amount of property arrays due to
middlewareInstances
. Snapshot were taken after disposing a player once, and then after instantiating/disposing e.g. 10 times.Adding logpoints in
tech/middleware.js
, we noticed that:getOrCreateFactory
is never called, so no new entry is set inmiddlewareInstances
for a new playerclearCacheForPlayer
is called every time player is disposed, so a new, null-valued entry is added tomiddlewareInstances
This happens for videojs v7.20.3. Comparing file with latest revision showed no significant change, so maybe the behaviour is the same in latest code.
If everything is correct, would you consider a tiny PR that addresses this? E.g.
clearCacheForPlayer
could check if key exists in the first place?Thanks
Reduced test case
None. Willing to create one if issue is of interest
Steps to reproduce
videoPlayer = videojs(videoElem, videoPlayerOptions);
and set a source on it withvideoPlayer.src({src: ..., type: ...})
videoPlayer.dispose();
Errors
None
What version of Video.js are you using?
7.20.3
Video.js plugins used.
None
What browser(s) including version(s) does this occur with?
Chrome 123
What OS(es) and version(s) does this occur with?
Windows 11
The text was updated successfully, but these errors were encountered: