Issue Details (XML | Word | Printable)

Key: JSR_333-74
Type: Bug Bug
Status: Open Open
Priority: Major Major
Assignee: Unassigned
Reporter: jukka.zitting
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
jsr-333

Potential deadlock in ObservationManager.removeEventListener()

Created: 16/Dec/13 02:51 PM   Updated: 16/Dec/13 04:00 PM
Component/s: api
Affects Version/s: current
Fix Version/s: None

Time Tracking:
Not Specified

Tags:
Participants: jukka.zitting and rhauch


 Description  « Hide

As discussed in https://issues.apache.org/jira/browse/OAK-1290, the ObservationManager.removeEventListener() method can lead to a deadlock because it "will block until the listener has completed executing." The deadlock occurs if the thread that calls this method holds a lock that the event listener is trying to acquire. The relevant javadoc paragraph is:

A listener may be deregistered while it is being executed. The deregistration method will block until the listener has completed executing. An exception to this rule is a listener which deregisters itself from within the onEvent method. In this case, the deregistration method returns immediately, but deregistration will effectively be delayed until the listener completes.

I see two ways to avoid this deadlock scenario:

  1. Keep the behavior as-is, but add documentation that informs clients about this problem: "... To avoid potential deadlocks, the caller of this method should not hold any locks that the event listener might try to acquire."
  2. Change the contract so that the method should not block indefinitely: "A listener may be deregistered while it is being executed. No more events will be delivered to the listener once this method has returned. An implementation should not block until the listener has completed executing. An exception to this rule is a listener which deregisters itself from within the onEvent method. In this case, the deregistration method returns immediately, but deregistration will effectively be delayed until the listener completes. To avoid potential deadlocks with implementations that might block, the caller of this method should not hold any locks that the event listener might try to acquire."

The first is fully backwards compatible, the second safer. I'm not sure if there are any clients out there that rely on the specified blocking behavior.



rhauch added a comment - 16/Dec/13 03:40 PM - edited

I agree that blocking doesn't really make much sense, especially because the phrase "until the listener has completed executing" is not clearly defined. What does it mean for a listener that is continually receiving additional lists of events (via the event iterator) to "complete executing".

But the proposed solutions still seem overly complicated.

Isn't the intention to simply require that a listener's "onEvent(EventIterator)" method should be atomic – once called with an iterator, the set of events as seen by the listener shouldn't be changed by deregistration. This includes the case when the listener deregisters itself from within an "onEvent" method.

My personal opinion is that calling "removeListener" should simply require that the specific listener's "onEvent" method will no longer be called once the listener is successfully deregistered. That definition is simple and is independent of who calls the method and what the listener might actually be doing at the time the method is called.

My proposal would be to change the behavior to no longer block, and to change the JavaDoc as follows:

Deregisters an event listener such that its <code>onEvent</code> no longer
be called.
<p>
This method has no effect if the listener is not registered at the time
this method is called.
</p>

@param listener The listener to deregister.
@throws RepositoryException If an error occurs.


jukka.zitting added a comment - 16/Dec/13 04:00 PM

I think the reasonable interpretation of "has completed executing" in this case is that no concurrent onEvent() call is in progress.

In general I'd also prefer to simplify the method to just stop any further onEvent() calls (with an option to also truncate an EventIterator that's currently being processed); that's what I was aiming at with option 2. However I'm a bit worried about making a change that would require potential changes in both clients and implementations of the API. My wording above was intended to allow existing implementations (that do block) to remain unmodified and for a new implementation to optionally block (triggered by some custom configuration) for such clients that still require that behavior for backwards compatibility.