Skip to content

Commit 42c98b3

Browse files
committed
FIXUP: Race condition in TarantoolClientImpl
Added comments and XXX notes.
1 parent 8d8aed9 commit 42c98b3

File tree

1 file changed

+41
-2
lines changed

1 file changed

+41
-2
lines changed

src/main/java/org/tarantool/TarantoolClientImpl.java

+41-2
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,21 @@ public class TarantoolClientImpl extends TarantoolBase<Future<?>> implements Tar
6565
protected Thread reader;
6666
protected Thread writer;
6767

68+
/**
69+
* The condition variable to signal a reconnection is needed from reader /
70+
* writer threads and waiting for that signal from the reconnection thread.
71+
*
72+
* The lock variable to access this condition.
73+
*
74+
* XXX: Maybe it is better to move signalling / waiting support to the
75+
* state helper (or fsm when we'll reimplement it in that way). Maybe
76+
* additional RECONNECTION_NEEDED state is required to distinguish whether
77+
* we're in the reconnection state already or just want to reconnect (don't
78+
* sure).
79+
*
80+
* @see #awaitReconnection()
81+
* @see #trySignalForReconnection()
82+
*/
6883
protected final ReentrantLock connectorLock = new ReentrantLock();
6984
protected final Condition reconnectRequired = connectorLock.newCondition();
7085

@@ -478,7 +493,7 @@ protected void stopIO() {
478493
}
479494

480495
/**
481-
* Blocks until a reconnection process can be carried on
496+
* Blocks until a reconnection signal will be received.
482497
*
483498
* @see #trySignalForReconnection()
484499
*/
@@ -496,7 +511,7 @@ private void awaitReconnection() {
496511
}
497512

498513
/**
499-
* Signals to the connector that reconnection process can be performed
514+
* Signals to the connector that reconnection process can be performed.
500515
*
501516
* @see #awaitReconnection()
502517
*/
@@ -675,28 +690,52 @@ protected int getState() {
675690
return state.get();
676691
}
677692

693+
/**
694+
* Set CLOSED state, drop RECONNECT state.
695+
*/
678696
protected boolean close() {
679697
for (; ; ) {
680698
int st = getState();
699+
700+
/* CLOSED is the terminal state. */
681701
if ((st & CLOSED) == CLOSED)
682702
return false;
703+
704+
/* Drop RECONNECT, set CLOSED.
705+
*
706+
* XXX: Should not we also drop other states? Or reimplement
707+
* this class as a finite state machine?
708+
*/
683709
if (compareAndSet(st, (st & ~RECONNECT) | CLOSED))
684710
return true;
685711
}
686712
}
687713

714+
/**
715+
* Move from a current state to a give one.
716+
*
717+
* Some moves are forbidden.
718+
*/
688719
protected boolean acquire(int mask) {
689720
for (; ; ) {
690721
int currentState = getState();
722+
723+
/* CLOSED is the terminal state. */
691724
if ((currentState & CLOSED) == CLOSED) {
692725
return false;
693726
}
727+
728+
/* Don't move to READING, WRITING or ALIVE from RECONNECT. */
694729
if ((currentState & RECONNECT) > mask) {
695730
return false;
696731
}
732+
733+
/* Cannot move from a state to the same state. */
697734
if ((currentState & mask) != 0) {
698735
throw new IllegalStateException("State is already " + mask);
699736
}
737+
738+
/* Set acquired state. */
700739
if (compareAndSet(currentState, currentState | mask)) {
701740
return true;
702741
}

0 commit comments

Comments
 (0)