We're updating the issue view to help you get more done. 

DefaultCache does not honour Map contract

Description

The DefaultCache implementation implements the Map interface. There are three methods that do not correctly implement this interface;

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;

  1. 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.

  2. Update all three methods to return a collections that align with Map documentation. NB. This is a non-trivial exercise.

  3. Replace the DefaultCache with a Caffeine Cache - https://github.com/ben-manes/caffeine which supports these methods as documented.

 

 

Environment

None

Acceptance Test - Entry

None

Activity

Show:
Greg Thomas
January 21, 2019, 10:32 PM

My preferences in order;

  1. 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).

  2. Barring that, I think returning unmodifiable collections is the way forward. It's not ideal, but implementing them properly is going to be hard.

Guus der Kinderen
January 22, 2019, 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 22, 2019, 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

Dele Olajide
January 22, 2019, 9:53 AM

What if we made the clustering plugin part of Openfire core and used ClusteredCache consistently in Openfire?

Greg Thomas
January 23, 2019, 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.

Assignee

Greg Thomas

Reporter

Greg Thomas

Labels

None

Expected Effort

None

Ignite Forum URL

None

Components

Fix versions

Affects versions

Priority

Major
Configure