Clear cache on reload #11673
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,32 @@ | ||||||||
| import Standard.Base.Data.Boolean.Boolean | ||||||||
| import Standard.Base.Nothing.Nothing | ||||||||
| import Standard.Base.Runtime.Managed_Resource.Managed_Resource | ||||||||
| import Standard.Base.Runtime.Ref.Ref | ||||||||
|
|
||||||||
| ## PRIVATE | ||||||||
| This is used by ReloadDetector.java to create a `Managed_Resource` that is | ||||||||
| finalized when the reload button is pressed. | ||||||||
|
|
||||||||
| The `on_finalize` function and the `clear` method both write `Nothing` to the | ||||||||
| ref. This is a signal that a reload has happenend. `on_finalize` is called by | ||||||||
| the engine when a reload happens. `clear` is only for testing, to simulate a | ||||||||
| reload. | ||||||||
|
|
||||||||
| The `0` value stored in the ref is not used; it just has to be something | ||||||||
| other than Nothing. | ||||||||
| type Reload_Detector | ||||||||
| private Value mr:Managed_Resource | ||||||||
|
|
||||||||
| new -> Reload_Detector = | ||||||||
| ref = Ref.new 0 | ||||||||
| on_finalize ref = ref.put Nothing | ||||||||
| mr = Managed_Resource.register ref on_finalize Boolean.True | ||||||||
| Reload_Detector.Value mr | ||||||||
|
|
||||||||
| get self = self.mr.with .get | ||||||||
|
|
||||||||
| clear self = | ||||||||
|
There was a problem hiding this comment.
|
||||||||
| self.mr.with (ref-> ref.put Nothing) | ||||||||
|
There was a problem hiding this comment. Given the most recent changes in the implementation this code shouldn't set the There was a problem hiding this comment. I am using a special value for the test-only implementation. |
||||||||
| Nothing | ||||||||
|
There was a problem hiding this comment. So that's why I was asking to have a Panic and not a dataflow error. If by any chance (through correct or incorrect code) the
Suggested change
This would have been much simpler if this was a Panic in the first place... (cc: @JaroslavTulach) There was a problem hiding this comment. In this case we however have a valid code.
sequence does. Maybe it was @GregoryTravis's intention? There was a problem hiding this comment. What can we learn from this? Let's claim: Use of There was a problem hiding this comment. It was a mistake -- I didn't realize the resource was GC'd as well as having the finalizer run. I don't think it's enough to check for Nothing -- what if something changes, and the Nothing is stored before the release of the reference, then it will no longer be nothing. There was a problem hiding this comment. I have updated it to use |
||||||||
|
|
||||||||
| create_reload_detector = Reload_Detector.new | ||||||||
GregoryTravis marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,6 +73,9 @@ public class LRUCache<M> { | |
| /** Used to get the current free disk space; mockable. */ | ||
| private final DiskSpaceGetter diskSpaceGetter; | ||
|
|
||
| /** Used to clear the cache on reload. */ | ||
| private final ReloadDetector reloadDetector = new ReloadDetector(); | ||
|
|
||
| public LRUCache() { | ||
| this(LRUCacheSettings.getDefault(), new NowGetter(), new DiskSpaceGetter()); | ||
| } | ||
|
|
@@ -89,6 +92,8 @@ public LRUCache(LRUCacheSettings settings, NowGetter nowGetter, DiskSpaceGetter | |
| */ | ||
| public CacheResult<M> getResult(ItemBuilder<M> itemBuilder) | ||
| throws IOException, InterruptedException, ResponseTooLargeException { | ||
| clearOnReload(); | ||
|
|
||
| String cacheKey = itemBuilder.makeCacheKey(); | ||
|
|
||
| try { | ||
|
|
@@ -221,6 +226,12 @@ public void clear() { | |
| removeCacheEntriesByPredicate(e -> true); | ||
| } | ||
|
|
||
| private void clearOnReload() { | ||
| if (reloadDetector.hasReloadOccurred()) { | ||
| clear(); | ||
| } | ||
| } | ||
|
|
||
| /** Remove all cache entries (and their cache files) that match the predicate. */ | ||
| private void removeCacheEntriesByPredicate(Predicate<CacheEntry<M>> predicate) { | ||
| List<Map.Entry<String, CacheEntry<M>>> toRemove = | ||
|
|
@@ -344,6 +355,11 @@ public LRUCacheSettings getSettings() { | |
| return settings; | ||
| } | ||
|
|
||
| /** Public for testing. */ | ||
| public void simulateReloadTestOnly() { | ||
| reloadDetector.simulateReloadTestOnly(); | ||
| } | ||
|
Comment on lines
+358
to
+361
There was a problem hiding this comment. I'm not convinced about this mock. After all - tests relying on this mock use completely different logic for checking if the cache should be cleared than what will be running in the IDE. The I think it would be best to add a test to the JVM tests, similar to something like There was a problem hiding this comment. I think perhaps we may want to proceed with this PR, as it's needed for release, with just manual testing. But I think it would be good to have some test that ensures that this works as expected. There was a problem hiding this comment.
Logic of If we have a unit test of There was a problem hiding this comment. My point was that we are not testing the integration between The problem is clear in the There was a problem hiding this comment. I agree that a test like this is necessary -- precisely because I have already had the tests pass but the feature fail to work. Thus: #11792. |
||
|
|
||
| private record CacheEntry<M>(File responseData, M metadata, long size, ZonedDateTime expiry) {} | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package org.enso.base.cache; | ||
|
|
||
| import org.enso.base.polyglot.EnsoMeta; | ||
| import org.graalvm.polyglot.Value; | ||
|
|
||
| /** | ||
| * Detects that the reload button has been pressed. | ||
| * | ||
| * <p>.hasReloadOccurred() returns true if the reload button was pressed since the last call to | ||
| * .hasReloadOccurred(). | ||
| * | ||
| * <p>This uses a weak reference (created in eval'd Enso code) that is set to null on reload. | ||
| */ | ||
| public class ReloadDetector { | ||
| // Weak reference that is set to null on reload. | ||
| private Value trigger; | ||
|
There was a problem hiding this comment. Is the comment still up-to-date? This contains a |
||
|
|
||
| public ReloadDetector() { | ||
| resetTrigger(); | ||
| } | ||
|
|
||
| public boolean hasReloadOccurred() { | ||
| var reloadHasOccurred = trigger.invokeMember("get").isNull(); | ||
|
There was a problem hiding this comment. This code was correct, but it is not anymore. In the IDE the value returned from E.g. one should check for |
||
| if (reloadHasOccurred) { | ||
| resetTrigger(); | ||
| } | ||
| return reloadHasOccurred; | ||
| } | ||
|
|
||
| private void resetTrigger() { | ||
| trigger = | ||
| EnsoMeta.callStaticModuleMethod( | ||
| "Standard.Base.Network.Reload_Detector", "create_reload_detector"); | ||
| } | ||
|
|
||
| void simulateReloadTestOnly() { | ||
| trigger.invokeMember("clear"); | ||
| } | ||
| } | ||
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.
Won't this fail if the
Managed_Resourceis cleaned, becausewithwill apply.getwithUninitialized_State? It feels like we should have acatchsomewhereThere 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.
Fixed.