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? |
src/js/tech/middleware.js
Outdated
| @@ -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.
There was a problem hiding this comment.
Let's go with delete here
There was a problem hiding this comment.
ok, commit is pushed
There was a problem hiding this comment.
not sure if - at this point - code should just delete without checking whether middlewareInstances has that property or not.
No, I tried re-running the failing test a few times. Upload to codecov is inexplicably flaky, even when a |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8674 +/- ##
=======================================
Coverage 83.01% 83.01%
=======================================
Files 119 119
Lines 7993 7994 +1
Branches 1923 1924 +1
=======================================
+ Hits 6635 6636 +1
Misses 1358 1358 ☔ View full report in Codecov by Sentry. |
|
Hi there. Just to let you know: if there's something more to do on our side, we might be able to. |
…t, delete cache key instead of setting to null
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