-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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(middleware): cache grows even if no middleware created #8674
base: main
Are you sure you want to change the base?
Conversation
…event cache from growing with null value
BTW, we're contributing to this on Windows, and still suffer from issue #8149. To make that work, we temporarily modified package.json |
We noticed from e-mails some repeated failure on that CI task. I guess, that is not related to the specific changes in code, is it? |
@@ -244,7 +244,9 @@ function executeRight(mws, method, value, terminated) { | |||
* A {@link Player} instance. | |||
*/ | |||
export function clearCacheForPlayer(player) { | |||
middlewareInstances[player.id()] = null; | |||
if (middlewareInstances.hasOwnProperty(player.id())) { | |||
middlewareInstances[player.id()] = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if delete
is preferable, if the object having null properties was the issue being addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the issue being addressed is that middlewareInstances
grows with new keys having null values. Not sure why the original code prefers setting to null instead of deleting.
Just as a speculation: in player.js
there's the global enumeration of players, Player.players
, and in that case it is explicitly mentioned that a disposed player should be left as a key with null value. Maybe that same behaviour has been applied in middlewareInstances
, even if there is no actual use of it.
No, I tried re-running the failing test a few times. Upload to codecov is inexplicably flaky, even when a |
Description
See issue #8653
Specific Changes proposed
When in
middleware.js
the functionclearCacheForPlayer
runs, before setting a value to null in middlware caches, it checks if the key exists in the first place.Requirements Checklist