Clojure JIRAhttp://dev.clojure.org/jira/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+CMEMOIZE+AND+resolution+%3D+Unresolved+AND+fixVersion+is+EMPTY+ORDER+BY+priority+DESC
An XML representation of a search requesten-us4.464925-07-2011[CMEMOIZE-15] ttl functions sometimes return nilhttp://dev.clojure.org/jira/browse/CMEMOIZE-15
core.memoize<p>If I (memoize/ttl) a function, near the threshold time the result from calling the memoized function will sometimes be nil.</p>
<p>I've attached a patch that exploits the problem in the unit tests.</p>
<p>Reproducing in the REPL is easy. Just call memoize/ttl fn a bunch of times and watch for nil results.</p>
<p>user=&gt; (require '<span class="error">&#91;clojure.core.memoize :as m&#93;</span>)<br/>
nil</p>
<p>user=&gt; (def f (m/ttl (constantly 1)))<br/>
#'user/f</p>
<p>user=&gt; (dotimes <span class="error">&#91;n 1000000&#93;</span> (when (nil? (f)) (println "nil f")))<br/>
nil f<br/>
nil f<br/>
nil f<br/>
nil f<br/>
nil f<br/>
nil</p>
<p>The problem seems to be that when clojure.core.cache/through gets called, the item hasn't expired, but when clojure.core.cache/lookup happens in build-memoizer, the expiration has passed.</p>CMEMOIZE-15ttl functions sometimes return nilDefectMajorOpenUnresolvedFogusRyan FowlerTue, 1 Jul 2014 16:24:42 -0500Wed, 17 Aug 2016 11:01:27 -050033<p>I'm out of my depth here, but this patch seems to resolve my problem. It looks in the cache before &amp; after the (through*) swap, picks the "after" value if non-nil and "before" value if the "after" value is nil.</p><p>One alternative would be to repeat the <div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<pre class="code-java">(swap! cache through* f args)</pre>
</div></div> if the value is nil.</p>
<p>Something like </p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<pre class="code-java">- (and val @val)))
+ (<span class="code-keyword">if</span> (nil? val)
+ (swap! cache through* f args)
+ @val)))</pre>
</div></div>
<p>There are a few benefits: less code, only expiration forces you to pay for an additional cache lookup, and it seems like it might be preferable semantically (you don't return a known-to-be-expired value). </p>
<p>A downside is that you may do multiple cache writes if the ttl is low enough (not sure if you can do sub-millisecond ttls?) or the thread scheduling is wonky enough to make that happen. But retries can happen with swap! regardless, so I'm not sure this drawback is significant.</p>
<p>p.s. You should also make sure to check out <a href="http://dev.clojure.org/display/community/Developing+Patches">http://dev.clojure.org/display/community/Developing+Patches</a> and use git format-patch for your patch, since the current ones don't have a `git am`-friendly format.</p><p>Isn't the problem the TTL cache impl itself? CacheProtocol is purely functional but TTLCache is not (has? or lookup are time sensitive). Cache-invalidation on write (rather than on read) may be a solution that wouldn't require changing the protocol.</p><p>Thanks Guys. </p>
<p>Chris, I think you're on to something. If TTLCache/lookup doesn't filter out results based on time, things make a whole lot more sense and the TTLCache acts more like normal Clojure.</p>
<p>(miss) and (evict) are the only functions that actually remove stuff from the cache, so one of those functions needs to get called regularly anyway.</p><p>Maybe I'm just being dense, but I'd personally be surprised if an implementation of CacheProtocol's lookup fn gave me back a value that should be expired according to the TTL. Imagine if your TTL was 30 seconds, and you did a read 10 minutes after your last write - calling lookup and getting a 10-minute-old value seems problematic.</p><p>Implementation of Colin Jones' suggestion to retry swap!/lookup when a cache lookup indicates that the cache entry doesn't exist.</p><p>We ran into this issue as well, and the temporary fix if anyone else needs it, is to call the memoized function a second time, which results in a cache miss and returns a new value.</p>
<p>@Colin nil is an unexpected value because in the case of a cache miss, the key's value will be repopulated. However, in this scenario, has? returns both true and false in the same function call.</p><p>For what it's worth, we've been running with CMEMOIZE&#45;15&#45;RETRY.diff in production for well over a year without issue. It's a shame that this hasn't been fixed in a proper release of either core.memoize or core.cache.</p><p>Unless I'm missing something, it seems like this patch hides the issue rather than fixes it? That doesn't seem like the right "fix".</p><p>I agree with Alex: the “retry if <tt>nil</tt>” strategy feels like a hack to me, although I think it’s the most appropriate workaround in the absence of an official fix.</p>
<p>Here’s an alternative approach:</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<pre class="code-none">diff --git a/src/main/clojure/clojure/core/memoize.clj b/src/main/clojure/clojure/core/memoize.clj
index 5b448eb..a18ebfe 100644
--- a/src/main/clojure/clojure/core/memoize.clj
+++ b/src/main/clojure/clojure/core/memoize.clj
@@ -159,7 +159,11 @@
(with-meta
(fn [&amp; args]
(let [cs (swap! cache through* f args)
- val (clojure.core.cache/lookup cs args)]
+ ;; Since `cache/through` (via `through*`) already
+ ;; handled hitting &amp; missing, treat the resulting `cs`
+ ;; as a “snapshot” and sidestep any call to `cache/lookup`.
+ val (get (.cache (.cache ^PluggableMemoization cs))
+ args)]
;; The assumption here is that if what we got
;; from the cache was non-nil, then we can dereference
;; it. core.memo currently wraps all of its values in</pre>
</div></div>
<p>With this change, all of the tests still pass, including the new test from the attached <a href="http://dev.clojure.org/jira/secure/attachment/13114/ttl_bug.diff">ttl_bug.diff</a>.</p>
<p>However, it’s more of a proof-of-concept than a viable fix (hence its living in this comment instead of being attached as a patch) because it introduces reflection (the outer <tt>.cache</tt>) by relying on the implementation detail that all of the cache implementations defined with <tt>defcache</tt> in <tt>clojure.core.cache</tt> have a raw backing map in a field named <tt>cache</tt>.</p>
<p>Of course, the reflection could be resolved with a new protocol method. I don’t yet have an opinion on whether it would be “better” to expand <tt>clojure.core.cache/CacheProtocol</tt> or to define a new protocol, nor if such a new protocol should live in <tt>clojure.core.cache</tt> or <tt>clojure.core.memoize</tt>.</p><p>Possible names for the prospective protocol method:</p>
<ul class="alternate" type="square">
<li><tt>raw-backing-map</tt></li>
<li><tt>backing-data</tt></li>
<li><tt>snapshot</tt>
<ul class="alternate" type="square">
<li>I am aware that core.memoize already defines its own <tt>snapshot</tt> function, but I don’t think there would be any problem if the protocol method lived in core.cache.</li>
</ul>
</li>
</ul>
<p>And for the protocol itself (assuming the method isn’t just added to <tt>clojure.core.cache/CacheProtocol</tt>):</p>
<ul class="alternate" type="square">
<li><tt>Transparence</tt></li>
<li><tt>Snapshottable</tt></li>
</ul>
<p><span class="nobr"><a href="http://dev.clojure.org/jira/secure/ViewProfile.jspa?name=alexmiller" class="user-hover" rel="alexmiller"><sup><img class="rendericon" src="http://dev.clojure.org/jira/images/icons/user_12.gif" height="12" width="12" align="absmiddle" alt="" border="0"/></sup>Alex Miller</a></span> (or <span class="nobr"><a href="http://dev.clojure.org/jira/secure/ViewProfile.jspa?name=fogus" class="user-hover" rel="fogus"><sup><img class="rendericon" src="http://dev.clojure.org/jira/images/icons/user_12.gif" height="12" width="12" align="absmiddle" alt="" border="0"/></sup>Fogus</a></span>, of course) any thoughts on <a href="http://dev.clojure.org/jira/browse/CMEMOIZE-15?focusedCommentId=43466&amp;page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-43466">the strategy described above</a>? Is it worth fleshing out?</p>Global RankPatchCode and Test[CMEMOIZE-9] memo-swap! is misnamedhttp://dev.clojure.org/jira/browse/CMEMOIZE-9
core.memoize<p>In order to be analogous to atoms, the current behavior of memo-swap! should be called memo-reset! since it overwrites the entire memo map. Similarly, there <b>should</b> be a function called memo-swap! that does an atomic swap operation on the memo cache so, for example, you can add a single value to the cache.</p>CMEMOIZE-9memo-swap! is misnamedEnhancementMinorOpenUnresolvedFogusMark EngelbergMon, 23 Sep 2013 17:38:18 -0500Mon, 23 Sep 2013 17:38:18 -050001Global Rank[CMEMOIZE-18] Confusing / erroneous documentation regarding seed values for clojure.core.memoize/memohttp://dev.clojure.org/jira/browse/CMEMOIZE-18
core.memoize<p>The documentation gives an example of passing a seed value to memo as a simple number. However, that does not work. The documentation is here: <a href="http://clojure.github.io/core.memoize/#clojure.core.memoize/memo">http://clojure.github.io/core.memoize/#clojure.core.memoize/memo</a></p>
<p>This does work:<br/>
((clojure.core.memoize/memo + {[42 1] (delay 99)}) 42 1)</p>
<p>This fails when it tries to deref 99:<br/>
((clojure.core.memoize/memo + {[42 1] 99}) 42 1)</p>JVM 1.7, 1.8. Clojure 1.6CMEMOIZE-18Confusing / erroneous documentation regarding seed values for clojure.core.memoize/memoDefectMinorOpenUnresolvedFogusMr. RmdocumentationmemoMon, 3 Nov 2014 11:14:20 -0600Mon, 3 Nov 2014 11:14:20 -060000Global Rank[CMEMOIZE-20] Option for snapshot to return a lazy sequencehttp://dev.clojure.org/jira/browse/CMEMOIZE-20
core.memoize<p>It'll be helpful to get a cache snapshot as a lazy sequence and thus avoiding having the whole cache and the snapshot in memory at once. As I'm working with big caches I've coded my own version of snapshot but would rather depend on a library function.</p>
<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<pre class="code-java">(defn snapshot
"Custom version of memo/snapshot that returns a lazy sequence of
[k v] pairs instead of an eagerly-built map."
[fn]
(when-let [cache (:clojure.core.memoize/cache (meta fn))]
(<span class="code-keyword">for</span> [[k v] (.cache ^clojure.core.memoize.PluggableMemoization @cache)]
[(vec k) @v])))</pre>
</div></div>Any.CMEMOIZE-20Option for snapshot to return a lazy sequenceEnhancementMinorOpenUnresolvedFogusArmando BlancasSun, 23 Aug 2015 13:41:16 -0500Fri, 6 Nov 2015 11:04:57 -060000Global Rank