fix(middleware): cache grows even if no middleware created #8674
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.
I wonder if delete is preferable, if the object having null properties was the issue being addressed
There was a problem hiding this comment.
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.jsthe functionclearCacheForPlayerruns, before setting a value to null in middlware caches, it checks if the key exists in the first place.Requirements Checklist