Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broadcasters lifecycle fails for non-string broadcaster Ids #1988

Open
silvpol opened this issue Jun 2, 2015 · 3 comments
Open

Broadcasters lifecycle fails for non-string broadcaster Ids #1988

silvpol opened this issue Jun 2, 2015 · 3 comments

Comments

@silvpol
Copy link

silvpol commented Jun 2, 2015

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
Copy link
Member

jfarcand commented Jun 2, 2015

@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
Copy link
Author

silvpol commented Jun 2, 2015

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
Copy link
Author

silvpol commented Jun 3, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants