Clojure

NPE in clojure.lang.Delay/deref

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

Delays get into a corrupt state if an exception is thrown, allowing deref to behave differently on subsequent calls.

This can manifest in multiple ways, depending on the expression being delayed:

;; calling a local as a function causes an NPE inside clojure.lang.Delay
(let [f #(/ 1 0) d (delay (f))]
  [(try (force d) 
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<ArithmeticException java.lang.ArithmeticException: Divide by zero> 
 #<NullPointerException java.lang.NullPointerException>]
;; if nil is a valid value, this can cause subsequent forces to "succeed"
;; even though the first fails as it should
(let [x (java.util.Date.) d (delay (seq x))]
  [(try (force d) 
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date> 
 nil]

Cause: clojure.core/delay creates a ^:once function, promising to call it only once, so that the compiler can do locals-clearing on its lexical closure. However, when an exception is thrown the Delay object is left thinking it has never called the function, so when it is forced again it calls the function again, breaking its promise to the compiler and causing the function to run after its locals have been cleared.

Solution: Make Delay remember the first exception that was thrown, and rethrow it on subsequent force attempts. This makes sense, in a way: the "result" of the expression was this exception e being thrown, and so this should happen every time.

Patch: delayed-exceptions.patch

Activity

Stuart Halloway made changes -
Field Original Value New Value
Approval Triaged [ 10120 ]
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Rich Hickey made changes -
Fix Version/s Release 1.6 [ 10157 ]
Stuart Halloway made changes -
Assignee Stuart Halloway [ stu ]
Stuart Halloway made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description If a Delay wraps a function which throws an exception, then forcing that delay multiple times causes behavior which is, to me at least, surprising and unexpected: the first time it is forced, the expected exception is thrown; but after that it will behave as if all locals the expression refers to are nil. This can manifest in multiple ways, depending on the expression being delayed:

{code}
;; calling a local as a function causes an NPE inside clojure.lang.Delay
(let [f #(/ 1 0) d (delay (f))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<ArithmeticException java.lang.ArithmeticException: Divide by zero>
 #<NullPointerException java.lang.NullPointerException>]
{code}

{code}
;; if nil is a valid value, this can cause subsequent forces to "succeed"
;; even though the first fails as it should
(let [x (java.util.Date.) d (delay (seq x))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date>
 nil]
{code}

The reason for this is that clojure.core/delay creates a ^:once function, promising to call it only once, so that the compiler can do locals-clearing on its lexical closure. However, when an exception is thrown the Delay object is left thinking it has never called the function, so when it is forced again it calls the function again, breaking its promise to the compiler and causing the function to run after its locals have been cleared.

In fact, once we realize that locals-clearing is involved, we can make the delay behave differently the first N times it is forced, instead of only the first time, by constructing an expression which throws an exception before using all of its locals:

{code}
(let [x (java.util.Date.)
            y (Long. 1)
            d (delay (let [y (seq y)]
                       (cons (seq x) y)))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.lang.Long>
 #<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date>
 (nil)]
{code}

I'm not sure what the right fix for this issue is: perhaps the best choice is even to leave it alone, and just document that delay's behavior is undefined if the expression throws an exception. However, I propose making the Delay remember the first exception that was thrown, and rethrow it on subsequent force attempts. This makes sense, in a way: the "result" of the expression was this exception *e* being thrown, and so this should happen every time. It might be preferable to have the Delay retry the expression until it succeeds, but I don't believe this is possible without abandoning locals-clearing, which would cause perfectly-valid delays to now run out of memory, holding onto no-longer-needed locals just in case the expression needs to retry at some later date.

So I've attached a patch that causes Delay to rethrow the original exception, and a test for that behavior, which of course fails if the change to Delay is not made.
Summary: Delays get into a corrupt state if an exception thrown, allowing deref to behave differently on back-to-back calls. Patch causes Delay to rethrow the original exception, includes tests.

If a Delay wraps a function which throws an exception, then forcing that delay multiple times causes behavior which is, to me at least, surprising and unexpected: the first time it is forced, the expected exception is thrown; but after that it will behave as if all locals the expression refers to are nil. This can manifest in multiple ways, depending on the expression being delayed:

{code}
;; calling a local as a function causes an NPE inside clojure.lang.Delay
(let [f #(/ 1 0) d (delay (f))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<ArithmeticException java.lang.ArithmeticException: Divide by zero>
 #<NullPointerException java.lang.NullPointerException>]
{code}

{code}
;; if nil is a valid value, this can cause subsequent forces to "succeed"
;; even though the first fails as it should
(let [x (java.util.Date.) d (delay (seq x))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date>
 nil]
{code}

The reason for this is that clojure.core/delay creates a ^:once function, promising to call it only once, so that the compiler can do locals-clearing on its lexical closure. However, when an exception is thrown the Delay object is left thinking it has never called the function, so when it is forced again it calls the function again, breaking its promise to the compiler and causing the function to run after its locals have been cleared.

In fact, once we realize that locals-clearing is involved, we can make the delay behave differently the first N times it is forced, instead of only the first time, by constructing an expression which throws an exception before using all of its locals:

{code}
(let [x (java.util.Date.)
            y (Long. 1)
            d (delay (let [y (seq y)]
                       (cons (seq x) y)))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.lang.Long>
 #<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date>
 (nil)]
{code}

I'm not sure what the right fix for this issue is: perhaps the best choice is even to leave it alone, and just document that delay's behavior is undefined if the expression throws an exception. However, I propose making the Delay remember the first exception that was thrown, and rethrow it on subsequent force attempts. This makes sense, in a way: the "result" of the expression was this exception *e* being thrown, and so this should happen every time. It might be preferable to have the Delay retry the expression until it succeeds, but I don't believe this is possible without abandoning locals-clearing, which would cause perfectly-valid delays to now run out of memory, holding onto no-longer-needed locals just in case the expression needs to retry at some later date.
Hide
Alex Miller added a comment -

Cleaned up description.

Show
Alex Miller added a comment - Cleaned up description.
Alex Miller made changes -
Description Summary: Delays get into a corrupt state if an exception thrown, allowing deref to behave differently on back-to-back calls. Patch causes Delay to rethrow the original exception, includes tests.

If a Delay wraps a function which throws an exception, then forcing that delay multiple times causes behavior which is, to me at least, surprising and unexpected: the first time it is forced, the expected exception is thrown; but after that it will behave as if all locals the expression refers to are nil. This can manifest in multiple ways, depending on the expression being delayed:

{code}
;; calling a local as a function causes an NPE inside clojure.lang.Delay
(let [f #(/ 1 0) d (delay (f))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<ArithmeticException java.lang.ArithmeticException: Divide by zero>
 #<NullPointerException java.lang.NullPointerException>]
{code}

{code}
;; if nil is a valid value, this can cause subsequent forces to "succeed"
;; even though the first fails as it should
(let [x (java.util.Date.) d (delay (seq x))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date>
 nil]
{code}

The reason for this is that clojure.core/delay creates a ^:once function, promising to call it only once, so that the compiler can do locals-clearing on its lexical closure. However, when an exception is thrown the Delay object is left thinking it has never called the function, so when it is forced again it calls the function again, breaking its promise to the compiler and causing the function to run after its locals have been cleared.

In fact, once we realize that locals-clearing is involved, we can make the delay behave differently the first N times it is forced, instead of only the first time, by constructing an expression which throws an exception before using all of its locals:

{code}
(let [x (java.util.Date.)
            y (Long. 1)
            d (delay (let [y (seq y)]
                       (cons (seq x) y)))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.lang.Long>
 #<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date>
 (nil)]
{code}

I'm not sure what the right fix for this issue is: perhaps the best choice is even to leave it alone, and just document that delay's behavior is undefined if the expression throws an exception. However, I propose making the Delay remember the first exception that was thrown, and rethrow it on subsequent force attempts. This makes sense, in a way: the "result" of the expression was this exception *e* being thrown, and so this should happen every time. It might be preferable to have the Delay retry the expression until it succeeds, but I don't believe this is possible without abandoning locals-clearing, which would cause perfectly-valid delays to now run out of memory, holding onto no-longer-needed locals just in case the expression needs to retry at some later date.
Delays get into a corrupt state if an exception is thrown, allowing deref to behave differently on subsequent calls.

This can manifest in multiple ways, depending on the expression being delayed:

{code}
;; calling a local as a function causes an NPE inside clojure.lang.Delay
(let [f #(/ 1 0) d (delay (f))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<ArithmeticException java.lang.ArithmeticException: Divide by zero>
 #<NullPointerException java.lang.NullPointerException>]
{code}

{code}
;; if nil is a valid value, this can cause subsequent forces to "succeed"
;; even though the first fails as it should
(let [x (java.util.Date.) d (delay (seq x))]
  [(try (force d)
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date>
 nil]
{code}

*Cause:* {{clojure.core/delay}} creates a {{^:once}} function, promising to call it only once, so that the compiler can do locals-clearing on its lexical closure. However, when an exception is thrown the Delay object is left thinking it has never called the function, so when it is forced again it calls the function again, breaking its promise to the compiler and causing the function to run after its locals have been cleared.

*Solution:* Make {{Delay}} remember the first exception that was thrown, and rethrow it on subsequent force attempts. This makes sense, in a way: the "result" of the expression was this exception *e* being thrown, and so this should happen every time.

*Patch:* {{delayed-exceptions.patch}}
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (3)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: