Broadcastable.broadcast() does not have timeout

[lyi2012]

The following method implementation does not have a timeout and causes the thread hanging forever in case the underneath future does not return as expected.

It will be nice to provide a timeout and potential a re-try logic in this unexpected case.

public Object broadcast() {
    try {
        return b.broadcast(message).get();
    } catch (Exception ex) {
        logger.error("failed to broadcast message: " + message, ex);
    }
    return null;
}

[jfarcand]

Use for `broadcaster.broadcast(..) retuns a Future, so use the appropriate .get(). Is it a method in Atmosphere you are referring to?

[lyi2012]

Hey, Jeanfrancois,

This is an Atmosphere method, but you are right, you did not use it internally in the latest code. I happen to find this public method and used it in my implementation, and did cause thread congestion. Just think it will be nice to remove this since another person like me may also run into the same issue.

[jfarcand]

I would be interested to learn how this is causing thread congestion...noirmally the thread will be unlocked once the message has been delivered.

[lyi2012]

Just noticed your comment here. A wild guess, should the "future" member be
volatile?

public static final class Entry {

    public Object message;

    public Object multipleAtmoResources;

    public BroadcasterFuture<?> future;

    public boolean writeLocally;

    public Object originalMessage;

Based on the JVM threading model, both threads have a reference to an Entry
object, enqueuing thread sets the "future" member, but the dequeuing
thread may still read it as null, if that happens, the following logic will
be skipped, countdown will never happen.

protected void entryDone(final BroadcasterFuture<?> f) {

    notifyBroadcastListener();

    if (f != null) f.done();

}

On Mon, Dec 9, 2013 at 7:10 AM, Jeanfrancois Arcand <
notifications@github.com> wrote:

I would be interested to learn how this is causing thread
congestion...noirmally the thread will be unlocked once the message has
been delivered.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1395#issuecomment-30139188
.

[lyi2012]

Never mind, it is a blockingqueue in the middle. But I suspect the initial countdown number might be an issue, it is based on the resources.size(). Can it be different from the actual countdowns?

See the code from 1.0.18,

public <T> Future<T> broadcast(T msg, AtmosphereResource r) {

    if (destroyed.get()) {
        logger.debug(DESTROYED, getID(), "broadcast(T msg, AtmosphereResource r");
        return futureDone(msg);
    }

    start();
    Object newMsg = filter(msg);
    if (newMsg == null) return futureDone(msg);

    BroadcasterFuture<Object> f = new BroadcasterFuture<Object>(newMsg, resources.size());
    dispatchMessages(new Entry(newMsg, r, f, msg));
    return f;
}