[JSR_333-74] Potential deadlock in ObservationManager.removeEventListener() Created: 16/Dec/13 Updated: 08/Sep/14 Resolved: 08/Sep/14
|Remaining Estimate:||Not Specified|
|Time Spent:||Not Specified|
|Original Estimate:||Not Specified|
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:
I see two ways to avoid this deadlock scenario:
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.
|Comment by rhauch [ 16/Dec/13 ]|
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:
|Comment by jukka.zitting [ 16/Dec/13 ]|
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.
|Comment by Peeter Piegaze [ 08/Sep/14 ]|
We will have to leave this to be interpreted rationally by implementers.