core.cache

defcache needs to impl. iterator() to avoid Clojure 1.7 breakage

Details

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

Description

After upgrading to Clojure 1.7 beta2 some of our code started failing, specifically attempts to use iterator() on (keys our-cache).

This is because post http://dev.clojure.org/jira/browse/CLJ-1499, calls to iterator on Seqables optimize by delegating to underlying collection iterator. defcache implements IPersistentMap but not iterator therefore:

user=> (.iterator (keys tc))

AbstractMethodError clojure.core.cache.TTLCache.iterator()Ljava/util/Iterator;  clojure.lang.APersistentMap$KeySeq.iterator (APersistentMap.java:184)

Proposal: Make defcache implement Iterable (which is implied by implementing IPersistentMap). I implemented by invoking .iterator on the underlying base-field, however this brings up the issue of reflection (warnings then popped up). To fix, I applied meta to base-field to type-hint it to clojure.lang.IPersistentMap.

This seems implied by various checks for map? elsewhere but I am not certain this is the correct type hint. It could be as narrow as java.lang.Iterable to be sufficient, but it seemed better to make a broader statement here if that is correct.

Patch: ccache-41.patch

Activity

Hide
Alex Miller added a comment -

defcache creates a deftype that implements IPersistentMap, but does not implement Iterable/iterator, even though IPersistentMap extends Iterable, so I consider this broken.

Show
Alex Miller added a comment - defcache creates a deftype that implements IPersistentMap, but does not implement Iterable/iterator, even though IPersistentMap extends Iterable, so I consider this broken.
Hide
Alex Miller added a comment -

The relevant ticket is not CLJ-1499 but CLJ-1602.

Show
Alex Miller added a comment - The relevant ticket is not CLJ-1499 but CLJ-1602.
Hide
Thomas Engelschmidt added a comment -

Is there any fix planned?
or should core.cache be considered permanently broken for clojure 1.7 ?

Show
Thomas Engelschmidt added a comment - Is there any fix planned? or should core.cache be considered permanently broken for clojure 1.7 ?
Hide
Fogus added a comment -

Looks like a winner to me.

Show
Fogus added a comment - Looks like a winner to me.
Hide
Alex Miller added a comment -

Patch applied for next release.

Show
Alex Miller added a comment - Patch applied for next release.

People

Vote (3)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: