Skip to content

Commit

Permalink
frontend: events inotify fix deadlock
Browse files Browse the repository at this point in the history
Motivation:

The inotify plugin contains code that can trigger a lock-inversion
(ABBA) deadlock.  When triggered, this deadlock can have quite a big
impact, as the (dCache) message delivery thread and (potentially) all
Jetty threads are caught in it.

Modification:

Remove the mistaken method-level synchronisation that results in the
lock inversion.

Update javadoc to make deadlock avoidance strategy clear.

Result:

A bug is fixed in the frontend if the inotify events are used.  The
trigger is closing a subscription when events are being delivered.  When
triggered, this deadlock can result in frontend apparently freezing.

Target: master
Requires-notes: yes
Requires-book: no
Request: 6.0
Request: 5.2
Request: 5.1
Request: 5.0
Patch: https://rb.dcache.org/r/12188/
Acked-by: Lea Morschel
Acked-by: Tigran Mkrtchyan
  • Loading branch information
paulmillar committed Jan 30, 2020
1 parent 7d78c80 commit 5366aca
Showing 1 changed file with 19 additions and 1 deletion.
Expand Up @@ -33,6 +33,8 @@
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Required;

import javax.annotation.concurrent.GuardedBy;

import java.io.IOException;
import java.util.Collection;
import java.util.EnumMap;
Expand Down Expand Up @@ -85,6 +87,18 @@
* An EventStream that provides the client with events based on namespace
* activity. The dCache-internal events that this EventStream provides are
* closely modelled on the inotify(7) API, from Linux.
* <p>
* Care must be taken when entering the two monitors: Inotify and
* InotifySelection. To avoid lock inversions (ABBA-type deadlocks), we must
* guarantee that the Inotify monitor is <emph>NEVER</emph> entered after a
* thread has entered the InotifySelection monitor.
* <p>
* One way to achieve this is to always enter the Inotify monitor before
* entering the InotifySelection monitor. This is sufficient to guarantee
* the code is free of such ABBA deadlocks, but this isn't strictly necessary.
* If the code can guarantee that the Inotify monitor is never entered then it
* is safe to enter the InotifySelection monitor without first entering the
* Inotify monitor.
*/
public class Inotify implements EventStream, CellMessageReceiver,
CuratorFrameworkAware, CellIdentityAware, CellLifeCycleAware
Expand Down Expand Up @@ -158,11 +172,13 @@ public synchronized void requestClose()
}

@Override
public synchronized void close()
public void close()
{
boolean justClosed;

synchronized (this) {
/* IMPORTANT: must NOT enter the Inotify monitor while inside
* this InotifySelection monitor. */
justClosed = !isClosed;

if (justClosed) {
Expand All @@ -184,8 +200,10 @@ public synchronized void close()
}
}

@GuardedBy("this")
private void sendEvent(JsonNode json)
{
/* MUST NOT cause this thread to enter the Inotify monitor. */
eventReceiver.accept(identity.selectionId(), json);
}

Expand Down

0 comments on commit 5366aca

Please sign in to comment.