Skip to content

Conversation

@jpatel531
Copy link
Contributor

So:

While an event queue (used for connecting, disconnecting, listener events) is shutting down, it still continues as it finishes its final tasks. While it is doing so, if a function that requires an event queue executes, it creates a new one.

This situation can lead to two event queues running at the same time. Issues #98 and #54 were, I believe, caused by an extremely subtle race condition where certain factors (e.g. custom reconnection logic, multiple instances of Pusher etc.) would cause multiple connection operations to be called at the same time. If a socket has been shut down - leading to an event queue being asked to shut down - and then there are multiple calls to connect executed on more than one event queue, you could get a race condition.

This led to inconsistencies regarding the connection state. If two threads see that that the state is DISCONNECTED, one gets there first and updates the state to CONNECTING and then the second thread tries to update the state to CONNECTING, we will get java.lang.IllegalArgumentException: Attempted to create an connection state update where both previous and current state are: CONNECTING.

In light of this, all Runnables put on the event queue are now wrapped in a synchronized block. The factory forever holds this lock and in the event there are two concurrent event queues, it will synchronize the Runnables.

cc @mdpye

@Override
public Object answer(InvocationOnMock invocation) throws Throwable {
final Runnable r = (Runnable) invocation.getArguments()[0];
new InstantExecutor().execute(r);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This InstantExecutor class is probably redundant now.

((Runnable)invocation.getArguments()[0]).run(); would do the trick.

To stick to proper OO patterns, we should really create a GloballySynchronizedExecutor which takes the lock object as a constructor arg, then we could still return a dummy type here.
Not going to insist on it though.

@jpatel531 jpatel531 force-pushed the connect-threadsafely branch from 346cee8 to fd33aab Compare March 23, 2016 16:28
@jpatel531 jpatel531 merged commit 16d5b89 into master Mar 23, 2016
@daniellevass daniellevass deleted the connect-threadsafely branch March 26, 2020 10:20
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

Successfully merging this pull request may close these issues.

3 participants