localStorage has no expiry of its own, and due to the implementation of CentralNotice' dismiss functionality not (fully) accounting for this, it seems to leave a lot of old entries for past central notices in the local storage. See this screenshot of my current list:
Ref T121646.
The kvStore does store expiry times (defaulting to 1/2year), but these just causes the data to be ignored once it has elapsed, and nothing gets deleted.
This could be resolved using actual expiry as implemented in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/804591/
Hi all! Thanks for this! :)
Unless there's a bug, this is incorrect. CentralNotice does delete expired entries from LocalStorage.
Here's the actual deletion, and here's where it's called from the CentralNotice startup module.
IIRC it may take a few return visits to the site for expired items to actually be cleaned up. Pretty sure it was done this way for performance reasons.
If you are able to verify that this does not happen correctly, then we should look into this as a bug (rather than a feature request).
Apologies also if this is poorly documented in the code. Looking over it now, I think we should at least add an inline comment explaining stuff here.
Thanks again!!
Ah, I see that the clearing happens in s separate file.
I think a lot of the queueing and idle callback stuff is probably unnecessary given the actual speeds involved in setting localStorage keys.
But also in my approach the expiries are stored in a separate object so we don't have to iterate over every key in localStotage, just the ones we know will expire eventually.
Hopefully this approach will be good enough for CN to be able to drop the expiry code.
> Hopefully this approach will be good enough for CN to be able to drop the expiry code.
Sounds quite reasonable! Definitely agreed there should be a unified approach throughout the stack. :)
Just a note, in case it's relevant: in its current incarnation, CentralNotice banner selection code should run as fast as possible. However, it's unclear to me whether or not CentralNotice will get a substantial refactor sometime soon; at least it's not impossible.
Thanks again!
I created https://www.measurethat.net/Benchmarks/Show/19293/1/localstoragesessionstorage-set-and-remove
On the slowest browser we support IE11, it can write a 1k value at 5,000 ops/sec and delete at 280,000 ops/sec, so nothing to worry about there.
Other browsers tested:
Looking then at @TheDJ 's original bug report, the items appear to have expiry dates, although they appear to be at least a year in the future.
Is the "bug" here that the expiry time is too long?