<< Back to previous view

[CMEMOIZE-8] memoizing functions that throw exceptions Created: 09/Jul/13  Updated: 06/Nov/15  Resolved: 06/Nov/15

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

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

core.memoize 0.5.6
clojure 1.5.1

Attachments: Text File core-memoize-race.patch    


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)
RuntimeException test-memo.core/print-and-throw (core.clj:6)
user> (test-memo.core/normal)
RuntimeException test-memo.core/print-and-throw (core.clj:6)

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)

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

This is caused by the following bug in delay;


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.

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)]
      (deref [this]
        (locking sentinel
          (let [val @val-ref]
            (if (identical? sentinel val)
              (let [new-val (f)]
                (reset! val-ref new-val)
Comment by Leon Barrett [ 09/Oct/15 6:18 PM ]

Here is a patch to fix the race condition as recommended by Jozef Wagner.

Comment by Leon Barrett [ 09/Oct/15 6:20 PM ]

I've submitted a patch to fix the last remaining little bit, the unsynchronized deref that causes the race condition. What else needs to be done for this to be merged into master?


Comment by Leon Barrett [ 16/Oct/15 1:05 PM ]

Hi. It's been a week. Can someone take a look at my patch? It would fix a longstanding issue.


Comment by Leon Barrett [ 02/Nov/15 5:28 PM ]

I've moved this race condition patch to a separate task: http://dev.clojure.org/jira/browse/CMEMOIZE-21

Comment by Alex Miller [ 06/Nov/15 10:58 AM ]

A version of this was implemented in core.memoize 0.5.7 with d-lay. There is a race condition in that implementation that is addressed in CMEMOIZE-21.

Generated at Wed Nov 25 02:20:54 CST 2015 using JIRA 4.4#649-r158309.