Clojure

Persistent assoc/conj on a transient-created collision node

Details

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

Description

Bug reported by Zach Tellman https://groups.google.com/d/msg/clojure-dev/HvppNjEH5Qc/1wZ-6qE7nWgJ

Since transients were introduced the invariant array.length == count*2 doesn't hold for HashCollisionNode.
However persistent .without still relies on it.

Hence persistent dissoc on a collision node created by transients fails.

(let [a (reify Object (hashCode [_] 42))
      b (reify Object (hashCode [_] 42))]
      (= (-> #{a b} transient (disj! a) persistent! (conj a))
       (-> #{a b} transient (disj! a) persistent! (conj a))))

returns false.

Patch: persistent-assoc-after-collision.diff

Generative test patch: transient-generative-test.diff

The generative test reliably reproduces the error. It is simpler than the original test that found the bug but tests a series conj/disj/transient/persistent actions on a set. I've included it separately in case we decide not to apply.

Screened by: Alex Miller

Activity

Hide
Alex Miller added a comment -

Patch was applied to master for 1.6.

Show
Alex Miller added a comment - Patch was applied to master for 1.6.
Hide
Alex Miller added a comment -

Added a simplified version of a test-generative test and marked screened.

Show
Alex Miller added a comment - Added a simplified version of a test-generative test and marked screened.
Hide
Alex Miller added a comment -

I'm going to take a crack at repro with test.generative this morning - wish me luck!

Show
Alex Miller added a comment - I'm going to take a crack at repro with test.generative this morning - wish me luck!
Hide
Michał Marczyk added a comment -

Just wanted to note that this patch, apart from preventing the hash-based collections from failing Zach's test suite, also makes avl.clj collections pass (now that I've released fixes for the two bugs uncovered by the test suite in avl.clj 0.0.9). This provides some cross-validation, I think.

(The built-in sorted collections pass either way, because they don't support transient ops.)

Also, David Nolen has merged the ClojureScript port of the patch.

Show
Michał Marczyk added a comment - Just wanted to note that this patch, apart from preventing the hash-based collections from failing Zach's test suite, also makes avl.clj collections pass (now that I've released fixes for the two bugs uncovered by the test suite in avl.clj 0.0.9). This provides some cross-validation, I think. (The built-in sorted collections pass either way, because they don't support transient ops.) Also, David Nolen has merged the ClojureScript port of the patch.
Hide
Andy Fingerhut added a comment -

Alex, I suspect clojure-dev would reach a much wider audience for your request than a comment on this ticket, which only has 3 watchers, and I don't think many people besides you and I watch the stream of all ticket state changes as they go by.

Show
Andy Fingerhut added a comment - Alex, I suspect clojure-dev would reach a much wider audience for your request than a comment on this ticket, which only has 3 watchers, and I don't think many people besides you and I watch the stream of all ticket state changes as they go by.
Hide
Alex Miller added a comment -

I don't suppose we could get a generative test (prob need to use test.generative which is already included) to test this stuff similar to the original test that found the bug?

Very much hoping to get this into 1.6.

Show
Alex Miller added a comment - I don't suppose we could get a generative test (prob need to use test.generative which is already included) to test this stuff similar to the original test that found the bug? Very much hoping to get this into 1.6.
Hide
Reid Draper added a comment -

I've run Zach's original test, as well as my own simple-check test. Both are passing.

Show
Reid Draper added a comment - I've run Zach's original test, as well as my own simple-check test. Both are passing.
Hide
Michał Marczyk added a comment -

I can confirm that the patch works for me. As per our #clojure conversation, I've done the ClojureScript port; see CLJ-648.

Show
Michał Marczyk added a comment - I can confirm that the patch works for me. As per our #clojure conversation, I've done the ClojureScript port; see CLJ-648.

People

Vote (4)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: