Cache.keySet() | values() | entrySet() .iterator().remove() throws UOE...

[mpokorny]

It would be advantageous if remove was supported as there no practical reason for not allowing this operation. I wrote a quick wrapper that tracks the last key given and does a cache.remove( lastKeyOut ) to verify what i already knew the cache *is* concurrent so removing is not a big deal.

I have included 3 tests w/ the thrown exception below each.

    public void testKeySetIterator() {

        final EmbeddedCacheManager manager = new DefaultCacheManager();

        final org.infinispan.Cache<String, String> cache = manager.getCache();

        final String key1 = "key1";

        final String key2 = "key2";

        final String key3 = "key3";

        final String value1 = "value1";

        final String value2 = "value2";

        final String value3 = "value3";

        cache.put(key1, value1);

        cache.put(key2, value2);

        cache.put(key3, value3);

        for (final Iterator<String> keys = cache.keySet().iterator(); keys.hasNext();) {

            keys.next();

            keys.remove();

        }

        cache.stop();

    }

java.lang.UnsupportedOperationException

    at org.infinispan.util.Immutables$ImmutableIteratorWrapper.remove(Immutables.java:283)

    at cache.infinispan.InfinispanCacheTest.testKeySetIterator(InfinispanCacheTest.java:89)

    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)

    public void testValuesIterator() {

        final EmbeddedCacheManager manager = new DefaultCacheManager();

        final org.infinispan.Cache<String, String> cache = manager.getCache();

        final String key1 = "key1";

        final String key2 = "key2";

        final String key3 = "key3";

        final String value1 = "value1";

        final String value2 = "value2";

        final String value3 = "value3";

        cache.put(key1, value1);

        cache.put(key2, value2);

        cache.put(key3, value3);

        for (final Iterator<String> values = cache.values().iterator(); values.hasNext();) {

            values.next();

            values.remove();

        }

        cache.stop();

    }

java.lang.UnsupportedOperationException

    at org.infinispan.util.Immutables$ImmutableIteratorWrapper.remove(Immutables.java:283)

    at cache.infinispan.InfinispanCacheTest.testValuesIterator(InfinispanCacheTest.java:112)

    public void testEntrySetIterator() {

        final EmbeddedCacheManager manager = new DefaultCacheManager();

        final org.infinispan.Cache<String, String> cache = manager.getCache();

        final String key1 = "key1";

        final String key2 = "key2";

        final String key3 = "key3";

        final String value1 = "value1";

        final String value2 = "value2";

        final String value3 = "value3";

        cache.put(key1, value1);

        cache.put(key2, value2);

        cache.put(key3, value3);

        for (final Iterator<Entry<String, String>> entries= cache.entrySet().iterator(); entries.hasNext();) {

            entries.next();

            entries.remove();

        }

        cache.stop();

    }

java.lang.UnsupportedOperationException

    at org.infinispan.util.Immutables$ImmutableIteratorWrapper.remove(Immutables.java:283)

    at cache.infinispan.InfinispanCacheTest.testEntrySetIterator(InfinispanCacheTest.java:135)

[mircea.markus]

Cache.keySet() returns a snapshot of all the keys in the local node only. IMO this method is rather useful for debugging purposes and not sure altering the state of the underlying cache is needed in this context.

[mpokorny]

But shouldnt Iterator.remove() still work and not throw a UOE ???

[mircea.markus]

This is not mandatory: jdk's immutable collections (and  cache.keySet() is a unmodifiable right now) do the same thing on iterator().remove().

[mpokorny]

But why make the iterator immutable, whats wrong with having a working Iterator.remove() ?

[mircea.markus]

It's not that we CAN'T. My point is that I'm not sure weather this would be very useful as Cache.keySet() returns a snapshot of the keys in the LOCAL cache.  IMO this is rather useful for debugging purposes, that's why a read-only iteration approach makes more sense to me - I don't have have strong feeling about it though    

[mpokorny]

Sure its a snapshot but,...how can it hurt to make Iterator.remove() work for the various Cache iterator views ? Its one thing that makes life a bit easier for users, without actually causing any harm.. Even if the value had disappeared by the time the Iterator.remove was invoked so what just let it fail silently.

[galder.zamarreno]

Miroslav, it's quite clear from the javadocs that the collections returned are immutable, so getting back an UOE is consistent with the javadocs. IIRC, having mutable iterators makes the code more complicated where you can simply do cache.remove(key). If you're interested in mutable iterators, maybe you could contribute a workable solution?

[mpokorny]

Yes the javadocs point this limitation out - everyone likes good javadocs

However back to the original suggestion - i have a very simple iterator wrapper with a working remove. For the moment it wraps a keys iterator, records the last given out key and when remove is called does a cache.remove( lastGivenOutKey). Because its --really small i have pasted it below. If you want tests i can supply those aswell. Having said all that expanding it make the other iterators for values and entryset to also work is and can be done using the same technique.

--wrapping iterator only--

/**

* An {@link Iterator} that stores the last key returned so remove now works.

*/

final class InfinispanCacheKeysIterator<K, V> implements Iterator<K> {

    static <K, V> InfinispanCacheKeysIterator<K, V> with(final Iterator<K> iterator, final Cache<K, V> cache) {

        return new InfinispanCacheKeysIterator<K, V>(iterator, cache);

    }

    private InfinispanCacheKeysIterator(final Iterator<K> iterator, final Cache<K, V> cache) {

        super();

        this.iterator = iterator;

        this.cache = cache;

    }

    @Override

    public boolean hasNext() {

        return this.iterator.hasNext();

    }

    /**

     * Stores the next key for possible use by {@link #remove()}

     */

    @Override

    public K next() {

        final K key = this.iterator.next();

        this.key = key;

        return key;

    }

    @Override

    public void remove() {

        final K key = this.key;

        if (null == key) {

            throw new IllegalStateException(REMOVE_BEFORE_NEXT);

        }

        final Cache<K, V> cache = this.cache;

        cache.remove(key);

        this.key = null;

    }

    static final String REMOVE_BEFORE_NEXT = "Cannot call remove before next";

    /**

     * The wrapped {@link Iterator keys iterator}

     */

    private final Iterator<K> iterator;

    private final Cache<K, V> cache;

    /**

     * The last key returned.

     */

    private K key;

    @Override

    public String toString() {

        return this.iterator.toString();

    }

}