You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by Michael Dürig <md...@apache.org> on 2013/06/14 11:23:30 UTC
Re: svn commit: r1492987 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/ oak-core/src/main/java/org/apache/jackrabbit/oak/spi/whiteboard/
oak-jcr/src/main/j...
Hi,
Below code breaks stop()'s contract, which states that no further events
will be delivered after calling stop. It will be that way probably most
of the time but not necessarily always.
Changing stop()'s contract escalates to
ObservationManagerImpl.removeEventListener, which explicitly states "The
deregistration method will block until the listener has completed
executing." It also escalates to Session.logout leading to the (rare but
confusing) possibility of events being still delivered to "logged out"
listeners.
Michael
On 14.6.13 8:51, jukka@apache.org wrote:
> @@ -108,29 +109,17 @@ class ChangeProcessor implements Runnabl
> * events will be delivered.
> * @throws IllegalStateException if not yet started or stopped already
> */
> - public synchronized void stop() {
> - if (future == null) {
> - throw new IllegalStateException("Change processor not started");
> - }
> -
> - try {
> - stopping = true;
> - future.cancel(true);
> - while (running) {
> - wait();
> - }
> - } catch (InterruptedException e) {
> - Thread.currentThread().interrupt();
> - }
> - finally {
> + public void stop() {
> + stopping = true; // do this outside synchronization
> + synchronized (this) {
> + checkState(registration != null, "Change processor not started");
> changeListener.dispose();
> - future = null;
> + registration.unregister();
> }
> }
Re: svn commit: r1492987 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/
oak-core/src/main/java/org/apache/jackrabbit/oak/spi/whiteboard/ oak-jcr/src/main/j...
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Fri, Jun 14, 2013 at 12:51 PM, Michael Dürig <md...@apache.org> wrote:
> Sounds good. I try to come up with something and change the expectation for
> the RepositoryTest.observationDispose test case.
OK, great!
BR,
Jukka Zitting
Re: svn commit: r1492987 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/ oak-core/src/main/java/org/apache/jackrabbit/oak/spi/whiteboard/
oak-jcr/src/main/j...
Posted by Michael Dürig <md...@apache.org>.
On 14.6.13 10:45, Jukka Zitting wrote:
> Hi,
>
> On Fri, Jun 14, 2013 at 12:41 PM, Michael Dürig <md...@apache.org> wrote:
>> AFAIU to comply with the removeEventListener contract we either have to
>> interrupt the observation thread (like my implementation did) or live with
>> the deadlock caused by client handlers blocking on events (like you
>> experienced with RepositoryTest.observationDispose).
>
> I would rather do the latter. Interrupting threads is nasty business,
Yes and clients usually just swallow the InterruptedException instead of
properly handling it and setting the thread's interrupted status...
which might lead to deadlocks as well.
> and the potential blocking would only affect the client that's in
> charge of the troublesome listener.
Sounds good. I try to come up with something and change the expectation
for the RepositoryTest.observationDispose test case.
Michael
Re: svn commit: r1492987 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/
oak-core/src/main/java/org/apache/jackrabbit/oak/spi/whiteboard/ oak-jcr/src/main/j...
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Fri, Jun 14, 2013 at 12:41 PM, Michael Dürig <md...@apache.org> wrote:
> AFAIU to comply with the removeEventListener contract we either have to
> interrupt the observation thread (like my implementation did) or live with
> the deadlock caused by client handlers blocking on events (like you
> experienced with RepositoryTest.observationDispose).
I would rather do the latter. Interrupting threads is nasty business,
and the potential blocking would only affect the client that's in
charge of the troublesome listener.
BR,
Jukka Zitting
Re: svn commit: r1492987 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/observation/ oak-core/src/main/java/org/apache/jackrabbit/oak/spi/whiteboard/
oak-jcr/src/main/j...
Posted by Michael Dürig <md...@apache.org>.
AFAIU to comply with the removeEventListener contract we either have to
interrupt the observation thread (like my implementation did) or live
with the deadlock caused by client handlers blocking on events (like you
experienced with RepositoryTest.observationDispose).
Michael
On 14.6.13 10:23, Michael Dürig wrote:
>
> Hi,
>
> Below code breaks stop()'s contract, which states that no further events
> will be delivered after calling stop. It will be that way probably most
> of the time but not necessarily always.
>
> Changing stop()'s contract escalates to
> ObservationManagerImpl.removeEventListener, which explicitly states "The
> deregistration method will block until the listener has completed
> executing." It also escalates to Session.logout leading to the (rare but
> confusing) possibility of events being still delivered to "logged out"
> listeners.
>
> Michael
>
>
> On 14.6.13 8:51, jukka@apache.org wrote:
>> @@ -108,29 +109,17 @@ class ChangeProcessor implements Runnabl
>> * events will be delivered.
>> * @throws IllegalStateException if not yet started or stopped
>> already
>> */
>> - public synchronized void stop() {
>> - if (future == null) {
>> - throw new IllegalStateException("Change processor not
>> started");
>> - }
>> -
>> - try {
>> - stopping = true;
>> - future.cancel(true);
>> - while (running) {
>> - wait();
>> - }
>> - } catch (InterruptedException e) {
>> - Thread.currentThread().interrupt();
>> - }
>> - finally {
>> + public void stop() {
>> + stopping = true; // do this outside synchronization
>> + synchronized (this) {
>> + checkState(registration != null, "Change processor not
>> started");
>> changeListener.dispose();
>> - future = null;
>> + registration.unregister();
>> }
>> }