Broadcasters lifecycle fails for non-string broadcaster Ids

[silvpol]

When Broadcasters are created with a String id everything seems to work normal, but when using arbitrary objects (UUID in our case) some of the lifecycle methods and DefaultBroadcasterFactory.store is not handled properly.

How to reproduce:
We're using AtmosphereHandler, so example uses that.
In GET part this code fails when last resource disconnects and then reconnects:

UUID eventId = UUID.fromString("28f99281-a4b4-11e3-a769-00231832fa86");
Broadcaster broadcaster = broadcasterFactory.lookup(eventId);
if (broadcaster == null) {
    broadcaster = broadcasterFactory.get(eventId);
    broadcaster.setBroadcasterLifeCyclePolicy(BroadcasterLifeCyclePolicy.EMPTY_DESTROY);
    broadcaster.getBroadcasterConfig().setBroadcasterCache(broadcasterCache);
}

But this code works fine:

UUID eventId = UUID.fromString("28f99281-a4b4-11e3-a769-00231832fa86");
Broadcaster broadcaster = broadcasterFactory.lookup(eventId.toString());
if (broadcaster == null) {
    broadcaster = broadcasterFactory.get(eventId.toString());
    broadcaster.setBroadcasterLifeCyclePolicy(BroadcasterLifeCyclePolicy.EMPTY_DESTROY);
    broadcaster.getBroadcasterConfig().setBroadcasterCache(broadcasterCache);
}

Internally in Atmosphere some lookups are done by broadcaster name rather than Id and either broadcaster isn't cleaned up correctly (Atmosphere 2.3.1) or 2 copies are kept in DefaultBroadcasterFactory.store, one keyed on String representation and the other on UUID object (Atmosphere 2.2.1).
When using String id following is logged when last resource disconnects:

DEBUG [2015-06-02 09:26:55,246] org.atmosphere.cpr.DefaultBroadcasterFactory: Removing Broadcaster 28f99281-a4b4-11e3-a769-00231832fa86 factory size now 2

When using UUID no message is logged and broadcatser stays in factory.

It seems to me that the whole issue boils down to the fact broadcaster name is used internally for lookups and String.toString() returns different object that UUID.toString() and ConcurrentHashMap inside DefaultBroadcasterFactory.store treats them as different objects.

Behaviour change between 2.2.1 and 2.3.1 seems to be due to fixes for #1878

[jfarcand]

@silvpol Great analysis. If you rollback 1878, does it work? In any case if you can share a test case that can help fixing the issue.

[silvpol]

Rolling back doesn't fix the issue, it just changes behaviour, the issue is present in both versions.

One of the places causing issue is signature of setID function in DefaultBroadcaster:

public synchronized void setID(String id) {
...
        Broadcaster b = config.getBroadcasterFactory().lookup(this.getClass(), id);
...
        config.getBroadcasterFactory().remove(this, name);
        this.name = id;
        config.getBroadcasterFactory().add(this, name);
..
}

For any non-string ID the first lookup will fail and the following lines will try to use name rather than ID
which is also not correct (I think).
To fix this I think the signature of setID would need to change to

public synchronized void setID(Object id) {

and any other related bits of code would need to be adjusted. I don't have enough undertanding of how framework works to change this, but happy to help with some coding.

I will try to create an example tonight.

[silvpol]

I've crated a simple example that allows to replicate the issue, I've published it on GitHub https://github.com/silvpol/atmosphere-issue-1988.
I'm not sure what your dev setup is like, but if you use IntelliJ you could just import as Gradle project. If you just want a quick play all you need is git clone and and run from commandline, all is in README.