<< Back to previous view

[CMEMOIZE-8] memoizing functions that throw exceptions Created: 09/Jul/13  Updated: 02/Jan/14

Status: In Progress
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: davie moston Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None
Environment:

core.memoize 0.5.6
clojure 1.5.1



 Description   

core.memoize doesn't retry if the orginal function throws an exception.
This is different to clojure.core/memoize, and looks like it's a result of using delays in core.memoize.
I think the core/memoize behaviour makes more sense - if an exception is thrown do not memoize the result, instead retry the function.
Example below.


(ns test-memo.core
(:require [clojure.core.memoize ])
(:import [java.lang.RuntimeException]))

(defn print-and-throw []
(do (println "hi") (throw (RuntimeException.))))

(def normal (memoize print-and-throw))
(def lru (clojure.core.memoize/lru print-and-throw))

user> (test-memo.core/normal)
hi
RuntimeException test-memo.core/print-and-throw (core.clj:6)
user> (test-memo.core/normal)
hi
RuntimeException test-memo.core/print-and-throw (core.clj:6)

(test-memo.core/lru)
hi
RuntimeException test-memo.core/print-and-throw (core.clj:6)
user> (test-memo.core/lru)
NullPointerException clojure.core.memoize/through*/fn-2771/fn-2772 (memoize.clj:53)



 Comments   
Comment by Martin Trojer [ 25/Jul/13 10:31 AM ]

This is caused by the following bug in delay;

http://dev.clojure.org/jira/browse/CLJ-1175

core.memoize probably shouldn't use delays, maybe revert to using atoms like (memoize).

The proposed solution in CLJ-1175 doesn't help in this case since we want to re-call the function causing the exception (again, just like memoize).

Comment by Fogus [ 13/Aug/13 8:22 AM ]

Version 0.5.7-SNAPSHOT has a potential fix in place. If you're so inclined to try it out I would appreciate any feedback that you have.
Thanks.

Comment by Jozef Wagner [ 02/Jan/14 4:46 PM ]

d-lay has an unsynchronized deref, which introduces a race condition between @memory and swap!. Moreover, memory seems to store only one element, so no map is needed there. I suggest something like:

(defn ^:private d-lay [f]
  (let [sentinel (Object.)
        val-ref (atom sentinel)]
    (reify
      clojure.lang.IDeref
      (deref [this]
        (locking sentinel
          (let [val @val-ref]
            (if (identical? sentinel val)
              (let [new-val (f)]
                (reset! val-ref new-val)
                new-val)
              val)))))))




[CMEMOIZE-14] dependency problem with clojure.core.cache Created: 26/Apr/14  Updated: 27/Apr/14

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Daniel Slutsky Assignee: Fogus
Resolution: Unresolved Votes: 1
Labels: None


 Description   

1. I created a simple project depending on clojure.core.memoize:
(defproject test-memoize "0.1.0-SNAPSHOT"
:description "FIXME: write description"
:url "http://example.com/FIXME"
:license {:name "Eclipse Public License"
:url "http://www.eclipse.org/legal/epl-v10.html"}
:dependencies [[org.clojure/clojure "1.5.1"]
[org.clojure/core.memoize "0.5.6"]])

2. Trying to use it runs into the following problem:
$ lein repl
nREPL server started on port 38522 on host 127.0.0.1
...
user=> (require 'clojure.core.memoize)

user=> CompilerException java.lang.RuntimeException: No such var: clojure.core.cache/through, compiling:(clojure/core/memoize.clj:52:3)

3. Such a problem has been recorded before:
https://groups.google.com/forum/#!msg/light-table-discussion/f4kpZLFGBV8/oyFPaJ4yvwwJ
http://www.raynes.me/logs/irc.freenode.net/clojure/2013-09-11.txt
https://github.com/LightTable/LightTable/issues/794

The suggested solution from the LightTable bug discussion
[[org.clojure/core.cache "0.6.3"]
[org.clojure/core.memoize "0.5.6" :exclusions [org.clojure/core.cache]]]
does not seem to work.



 Comments   
Comment by Daniel Slutsky [ 27/Apr/14 8:19 AM ]

When seeing the above error, I had
{:user {...
:dependencies [[leiningen "2.3.4"]
...]
...}}
at ~/.lein/profiles.clj .

After removing leiningen from the dependencies, the error disappeared.
Having Leiningen in the dependencies was required for using vinyasa (https://github.com/zcaudate/vinyasa).

The bottom line: can use clojure.core.memoize, cannot use it together with vinyasa.





[CMEMOIZE-15] ttl functions sometimes return nil Created: 01/Jul/14  Updated: 11/Jul/14

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ryan Fowler Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File fix-CMEMEMOIZE-15.diff     File ttl_bug.diff    
Patch: Code and Test

 Description   

If I (memoize/ttl) a function, near the threshold time the result from calling the memoized function will sometimes be nil.

I've attached a patch that exploits the problem in the unit tests.

Reproducing in the REPL is easy. Just call memoize/ttl fn a bunch of times and watch for nil results.

user=> (require '[clojure.core.memoize :as m])
nil

user=> (def f (m/ttl (constantly 1)))
#'user/f

user=> (dotimes [n 1000000] (when (nil? (f)) (println "nil f")))
nil f
nil f
nil f
nil f
nil f
nil

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.



 Comments   
Comment by Ryan Fowler [ 01/Jul/14 8:22 PM ]

I'm out of my depth here, but this patch seems to resolve my problem. It looks in the cache before & after the (through*) swap, picks the "after" value if non-nil and "before" value if the "after" value is nil.

Comment by Colin Jones [ 08/Jul/14 5:41 PM ]

One alternative would be to repeat the

(swap! cache through* f args)
if the value is nil.

Something like

-            (and val @val)))
+            (if (nil? val)
+              (swap! cache through* f args)
+              @val)))

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).

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.s. You should also make sure to check out http://dev.clojure.org/display/community/Developing+Patches and use git format-patch for your patch, since the current ones don't have a `git am`-friendly format.

Comment by Christophe Grand [ 10/Jul/14 4:50 AM ]

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.

Comment by Ryan Fowler [ 10/Jul/14 1:47 PM ]

Thanks Guys.

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.

(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.

Comment by Colin Jones [ 11/Jul/14 10:49 AM ]

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.





[CMEMOIZE-16] Reflection warning, clojure/core/memoize.clj:72:23 - reference to field cache can't be resolved. Created: 24/Jul/14  Updated: 19/Aug/14

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Daniel Ziltener Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Java 8, Clojure 1.6



 Description   

As the summary says. On the other hand it seems this never gets called. The .cache function isn't defined anywhere either.



 Comments   
Comment by Andy Fingerhut [ 19/Aug/14 6:23 PM ]

Appears to be a duplicate of ticket CMEMOIZE-13. That ticket includes a patch that eliminates the reflection warning.





[CMEMOIZE-17] bump core.cache dependency Created: 06/Aug/14  Updated: 06/Aug/14

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major
Reporter: Max Penet Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None


 Description   

https://github.com/clojure/core.cache#change-log
It brings a few improvements/bugfixes, an upgrade would be (very) welcomed.






[CMEMOIZE-9] memo-swap! is misnamed Created: 23/Sep/13  Updated: 23/Sep/13

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mark Engelberg Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None


 Description   

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 should 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.






[CMEMOIZE-10] Odd deprecation warning in face of correct API usage? Created: 29/Nov/13  Updated: 03/Dec/13

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Magnar Sveen Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: documentation, errormsgs
Environment:

[org.clojure/core.memoize "0.5.6"]



 Description   

Calling

(clojure.core.memoize/ttl get-optimized-assets {} :ttl/threshold ms)

Results in this

WARNING - Deprecated construction method for lu cache; prefered way is: (clojure.core.memoize/lu function <base> <:lu/threshold num>)

This breaks my expectation in two ways:

  • If I'm using `ttl` wrong, I would expect a deprecation warning for that. Now I'm a little stuck.
  • If I'm using `ttl` right, I wouldn't expect a deprecation warning.

Now I am just confused. Am I using a deprecated API? In that case, what is the right one to use? Looking at the source code, I see

Please use clojure.core.memoize/ttl instead.

Which is what I am using, as far as I can tell.

Maybe not a very important issue, but it has me confused and unsure.



 Comments   
Comment by Magnar Sveen [ 03/Dec/13 12:12 AM ]

Turns out I got bit by the JVMs global namespace. Once I included 0.5.6, the ring-middleware-format in our stack also started using that updated version - with the wrong API.

Is it only node.js that has this dependency thing figured out properly?

Sorry about the misleading issue. I'd close it, but I don't have the rights to do so.





[CMEMOIZE-11] Investigate why build errors occur for Clojure 1.2.0 on IBM JDK 1.6 Created: 29/Nov/13  Updated: 29/Nov/13

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Fogus Assignee: Fogus
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The failure report at http://build.clojure.org/job/core.memoize-test-matrix/71/ has more detail. Error messages refer to keyword interning. The dependency has been updated to Clojure 1.2.1 for now.






[CMEMOIZE-12] NPE with memoize/ttl Created: 05/Dec/13  Updated: 05/Jun/14

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Scott Morgan Assignee: Fogus
Resolution: Unresolved Votes: 1
Labels: bug
Environment:

org.clojure/core.memoize "0.5.6"



 Description   

I'm trying still trying to reproduce it, but I noted this in my logs if my compojure app on a subsequent request to the memoized function.

java.lang.NullPointerException
at clojure.core.memoize$through_STAR_$fn_401$fn_402.invoke(memoize.clj:53)
at clojure.lang.Delay.deref(Delay.java:33)
at clojure.core$deref.invoke(core.clj:2128)
at clojure.core.memoize$build_memoizer$fn__456.doInvoke(memoize.clj:140)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.lang.AFunction$1.doInvoke(AFunction.java:29)
at clojure.lang.RestFn.invoke(RestFn.java:436)



 Comments   
Comment by Aaron Iba [ 19/Feb/14 4:49 PM ]

I have the same exact stack trace / line numbers in my logs. I have no idea how to reproduce. I would love to get advice on how to debug this.

In my case, this is running in a web app (ring) context.

Comment by Nicola Mometto [ 19/Feb/14 6:13 PM ]

I noticed that core.memoize throws an NPE if the function memoized throws an exception.
I don't know if this is what you're running into but it might be something to look into.

Comment by Steve Losh [ 04/Jun/14 3:21 PM ]

We're seeing this too. To reproduce:

(require '[clojure.core.memoize :as memo])

(defn f []
  (/ 1 0))

(def m (memo/ttl f :ttl/threshold 5000))

If you call (m) the first time, you get a divide by zero exception as expected. If you call it again you'll get an NPE until the TTL expires (in 5 seconds in this case) at which point you'll get one correct error and then more NPEs.

Comment by Steve Losh [ 05/Jun/14 12:04 PM ]

Note that if you update to Clojure 1.6 the NPEs go away, but the memoizer caches the exception as the result of the function. This is fine for pure functions but not for what we need, so we just wrote a wrapper around memoize/ttl to catch exceptions and evict the cache before rethrowing them. It's ugly but it works good enough.





[CMEMOIZE-13] Reflection warning in clojure.core.memoize/snapshot Created: 14/Jan/14  Updated: 20/Mar/14

Status: Open
Project: core.memoize
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Trivial
Reporter: Miikka Koskinen Assignee: Fogus
Resolution: Unresolved Votes: 2
Labels: None
Environment:

core.memoize 0.5.6, Clojure 1.5.1


Attachments: Text File cmemoize-13-v1.patch    
Patch: Code

 Description   

When core.memoize is used with *warn-on-reflection* set to true:

Reflection warning, clojure/core/memoize.clj:72:23 - reference to field cache can't be resolved.


 Comments   
Comment by Andy Fingerhut [ 20/Mar/14 6:49 PM ]

Patch cmemoize-13-v1.patch eliminates the reflection in fn snapshot by type-hinting the value of @cache





Generated at Thu Oct 23 03:54:46 CDT 2014 using JIRA 4.4#649-r158309.