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

Improve StateMachine addStateChangeListener use and semantics #11974

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dain
Contributor

dain commented Nov 25, 2018

  • Fix this leaks when adding state change listeners in constructors
  • Immediately fire current state when a state change listener is added

dain added some commits Nov 24, 2018

Immediately fire current state in StateMachine addStateChangeListener
Always firing the current state simplifies state change listener implementation.

@dain dain requested a review from raghavsethi Nov 25, 2018

@@ -268,7 +271,7 @@ public void addStateChangeListener(StateChangeListener<T> stateChangeListener)
// state machine will never transition from a terminal state, so fire state change immediately
if (inTerminalState) {
// always fire listener callbacks from a different thread
// always fire listener callbacks from a different thread

This comment has been minimized.

@raghavsethi

raghavsethi Nov 29, 2018

Contributor

Typo?

@@ -263,6 +250,31 @@ private SqlTaskExecution(
}
}
// this is a separate method to ensure that the `this` reference is not leaked during construction

This comment has been minimized.

@raghavsethi

raghavsethi Nov 29, 2018

Contributor

// static method to ensure that the this reference is not leaked during construction

// this is a separate method to ensure that the `this` reference is not leaked during construction
private void initialize()

This comment has been minimized.

@raghavsethi

raghavsethi Nov 29, 2018

Contributor

Would recommend changing comment like above.

@@ -263,6 +250,31 @@ private SqlTaskExecution(
}
}
// this is a separate method to ensure that the `this` reference is not leaked during construction

This comment has been minimized.

@raghavsethi

raghavsethi Nov 29, 2018

Contributor

// static method to ensure that the this reference is not leaked during construction

@raghavsethi

Second commit looks good too.

@@ -255,27 +255,26 @@ private void fireStateChangedListener(T newState, StateChangeListener<T> stateCh
* Adds a listener to be notified when the state instance changes according to {@code .equals()}.
* Listener is always notified asynchronously using a dedicated notification thread pool so, care should
* be taken to avoid leaking {@code this} when adding a listener in a constructor. Additionally, it is
* possible notifications are observed out of order due to the asynchronous execution.
* possible notifications are observed out of order due to the asynchronous execution. The listener is
* immediately notified immediately of the current state.

This comment has been minimized.

@raghavsethi

raghavsethi Nov 29, 2018

Contributor
always notified immediately of the current state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment