-
Notifications
You must be signed in to change notification settings - Fork 138
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
Fixing Deadlock Bug Between ZK Callback & Event Thread On Acquiring Coordinator Object #964
Changes from all commits
7382a52
aa3069e
c8c4670
6139970
e1aca7a
d9a457a
d344473
87938c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -321,23 +321,30 @@ public void start() { | |
_heartbeatPeriod.toMillis() * 3, _heartbeatPeriod.toMillis(), TimeUnit.MILLISECONDS); | ||
} | ||
|
||
private synchronized void createEventThread() { | ||
protected synchronized void createEventThread() { | ||
_eventThread = new CoordinatorEventProcessor(); | ||
_eventThread.setDaemon(true); | ||
} | ||
|
||
private synchronized void startEventThread() { | ||
if (!_shutdown) { | ||
_eventThread.start(); | ||
CoordinatorEventProcessor eventThread = getEventThread(); | ||
eventThread.start(); | ||
} | ||
} | ||
|
||
private synchronized boolean stopEventThread() { | ||
// interrupt the thread if it's not gracefully shutdown | ||
while (_eventThread.isAlive()) { | ||
CoordinatorEventProcessor eventThread = getEventThread(); | ||
while (eventThread.isAlive()) { | ||
// Waits to acquire the Coordinator object for a maximum of _heartbeat period. | ||
// The time bound waiting prevents the caller thread to not infinitely wait if | ||
// the event thread is already shutdown. | ||
waitForNotificationFromEventThread(_heartbeatPeriod); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we wait for (_heartbeatPeriod - some delta) ? This comment is valid only of this thread is responsible for punching heartbeats. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only the coordinator thread is responsible for punching heartbeats. And we are never waiting on the coordinator thread. We are only waiting on any ZK callback threads or the main datastream server thread. |
||
try { | ||
_eventThread.interrupt(); | ||
_eventThread.join(EVENT_THREAD_SHORT_JOIN_TIMEOUT); | ||
_log.info("Attempting to interrupt the event thread."); | ||
eventThread.interrupt(); | ||
eventThread.join(EVENT_THREAD_SHORT_JOIN_TIMEOUT); | ||
} catch (InterruptedException e) { | ||
_log.warn("Exception caught while interrupting the event thread", e); | ||
return true; | ||
|
@@ -346,16 +353,40 @@ private synchronized boolean stopEventThread() { | |
return false; | ||
} | ||
|
||
// Waiting for the event thread to die. | ||
private synchronized boolean waitForEventThreadToJoin() { | ||
CoordinatorEventProcessor eventThread = getEventThread(); | ||
if (!eventThread.isAlive()) { | ||
return false; | ||
} | ||
// Waits to acquire the Coordinator object for a maximum of _heartbeat period. | ||
// The time bound waiting prevents the caller thread to not infinitely wait if | ||
// the event thread is already shutdown. | ||
waitForNotificationFromEventThread(_heartbeatPeriod); | ||
try { | ||
_eventThread.join(EVENT_THREAD_LONG_JOIN_TIMEOUT); | ||
_log.info("Waiting for {} milliseconds for the event thread to die.", EVENT_THREAD_LONG_JOIN_TIMEOUT); | ||
eventThread.join(EVENT_THREAD_LONG_JOIN_TIMEOUT); | ||
} catch (InterruptedException e) { | ||
_log.warn("Exception caught while waiting the event thread to stop", e); | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
// Waits for a notification for specified duration from the event thread before acquiring the Coordinator object. | ||
private synchronized void waitForNotificationFromEventThread(Duration duration) { | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there anyway to validate if the object lock is held when we enter this method (as we are calling wait here) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate that the wait releases the lock on the "this" object? Since its an intrinsic property of the wait function to release the lock on the object from where it is called, not sure if we need validation in code. In the tests, it should be validating that. I have tried reproducing the deadlock scenario in one of the tests and ensuring no deadlock would mean that the object lock is released when the zk session expiry thread is waiting. |
||
// This intrinsic conditional variable helps to halt threads (zk callback threads, main server thread) before | ||
// attempting to acquire the Coordinator object. We never halt the event thread (coordinator thread) | ||
// explicitly via this CV. | ||
_log.info("Thread {} will wait for notification from the event thread for {} ms.", | ||
Thread.currentThread().getName(), duration.toMillis()); | ||
this.wait(duration.toMillis()); | ||
} catch (InterruptedException exception) { | ||
_log.warn("Exception caught while waiting for the notification from the event thread", exception); | ||
} | ||
} | ||
|
||
/** | ||
* Stop coordinator (and all connectors) | ||
*/ | ||
|
@@ -2206,6 +2237,14 @@ private void populateDatastreamDestinationFromExistingDatastream(Datastream data | |
existingStream.getMetadata().get(DatastreamMetadataConstants.TASK_PREFIX)); | ||
} | ||
|
||
// Via the intrinsic conditional variable, notify other threads that might | ||
// be waiting on acquiring access on the Coordinator object. | ||
// We are only calling notify on the synchronized Coordinator Object's ("this") waiting threads. | ||
// Suppressing the Naked_Notify warning on this. | ||
protected synchronized void notifyThreadsWaitingForCoordinatorObjectSynchronization() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this helper method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to use this from couple of tests and hence decided to keep it in a generic function which can be reused. |
||
this.notifyAll(); | ||
} | ||
|
||
@Override | ||
public List<BrooklinMetricInfo> getMetricInfos() { | ||
return _metrics.getMetricInfos(); | ||
|
@@ -2296,7 +2335,7 @@ CachedDatastreamReader getDatastreamCache() { | |
return _datastreamCache; | ||
} | ||
|
||
private class CoordinatorEventProcessor extends Thread { | ||
protected class CoordinatorEventProcessor extends Thread { | ||
@Override | ||
public void run() { | ||
_log.info("START CoordinatorEventProcessor thread"); | ||
|
@@ -2305,6 +2344,7 @@ public void run() { | |
CoordinatorEvent event = _eventQueue.take(); | ||
if (event != null) { | ||
handleEvent(event); | ||
notifyThreadsWaitingForCoordinatorObjectSynchronization(); | ||
} | ||
} catch (InterruptedException e) { | ||
_log.warn("CoordinatorEventProcessor interrupted", e); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are we not pointing at _eventThread instead of calling getEventThread() ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an API makes it much better to test it with overrides instead of mocking the var, hence I changed the references to use the APIs to fetch the variable value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!