DefaultCache does not honour Map contract
Activity
Greg Thomas January 23, 2019 at 11:42 AM
I've plumped for option #2 for now. I'd still like to go for Caffeine, but that's hard.
I'm not sure using a Hazelcast IMap is a good idea; it means that even a single standalone Openfire has to run a Hazelcast cluster, which requires additional configuration that not every one will want to carry out.
Dele Olajide January 22, 2019 at 9:53 AM
What if we made the clustering plugin part of Openfire core and used ClusteredCache consistently in Openfire?
Greg Thomas January 22, 2019 at 9:44 AM
re. making the collections unmodifiable, we should be able to get away with just modifying CacheWrapper, which wraps both DefaultCache and ClusteredCache
Guus der Kinderen January 22, 2019 at 9:36 AM
I'm torn on this. I'd love for Openfire to do the 'right thing' (MAP API-wise) but I feel that resorting to to unmodifiable collections is a more pragmatic solution, which probably isn't going to hurt anyone. In any case, we should have the same behavior in clustered and non-clustered caches. If we do switch to unmodifiable, does that require changes in the hazelcast plugin?
Greg Thomas January 21, 2019 at 10:32 PM
My preferences in order;
I like Caffeine, and in an ideal world I think it would be a Good Thing (TM) to move to that - it's probably more performant than the home-brew solution used by Openfire, as well as more functional. However, it is sized by the number of entries, rather than the amount of RAM it uses as currently configured (Hazlecast can be configured either way).
Barring that, I think returning unmodifiable collections is the way forward. It's not ideal, but implementing them properly is going to be hard.
The DefaultCache implementation implements the Map interface. There are three methods that do not correctly implement this interface;
DefaultCache#values() - the returned collection does not honour any modifications, unlike the documented implementation at https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#values-- - the returned collection throws an UnsupportedOperationException on any attempts to modify
DefaultCache#entrySet() - the returned set will not throw an exception on any attempt to modify it, but will not honour the modifications in the parent Map as documented at https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#entrySet--
DefaultCache#keySet() - the returned set will not throw an exception on any attempt to modify it, but will not honour the modifications in the parent Map as documented at https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#keySet--
Note 1: The PR at https://github.com/igniterealtime/Openfire/pull/1235 (which also references https://issues.igniterealtime.org/browse/OF-867) highlights an bug that has been caused by an assumption that the DefaultCache supports the Map interface.
Note 2: ClusteredCache from the Hazelcast plugin does the right thing - by leveraging the Hazelcast implementation.
I can see three options;
Make the sets returned from entrySet() and keySet() read-only, and update the Javadoc for the Cache to reflect this state. Update the ClusteredCache similarly. Also double check the code base to find any incorrect use of these methods in addition to that highlighted in OF-867.
Update all three methods to return a collections that align with Map documentation. NB. This is a non-trivial exercise.
Replace the DefaultCache with a Caffeine Cache - https://github.com/ben-manes/caffeine which supports these methods as documented.