core.memoize

memoizing functions that throw exceptions

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • 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)

Activity

Hide
Alex Miller added a comment -

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.

Show
Alex Miller added a comment - 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.
Hide
Leon Barrett added a comment -

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

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

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

Thanks.

Show
Leon Barrett added a comment - Hi. It's been a week. Can someone take a look at my patch? It would fix a longstanding issue. Thanks.
Hide
Leon Barrett added a comment -

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?

Thanks.

Show
Leon Barrett added a comment - 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? Thanks.
Hide
Leon Barrett added a comment -

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

Show
Leon Barrett added a comment - Here is a patch to fix the race condition as recommended by Jozef Wagner.
Hide
Jozef Wagner added a comment -

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)))))))
Show
Jozef Wagner added a comment - 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)))))))
Hide
Fogus added a comment -

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.

Show
Fogus added a comment - 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.
Hide
Martin Trojer added a comment -

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

Show
Martin Trojer added a comment - 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).

People

Vote (0)
Watch (6)

Dates

  • Created:
    Updated:
    Resolved: