SynchronizableDocument implementation is deadlock friendly

[iloveeclipse]

Coming from eclipse-platform/eclipse.platform#282.

The SynchronizableDocument is supposed to be "thread safe" version of AbstractDocument for use in both UI and background tasks, in contrast to AbstractDocument, which is not thread safe.

The code in SynchronizableDocument adds locks around few AbstractDocument methods which modify document data and notify listeners afterwards about changes.

This implementation is a parade example of what one should NOT do for the safe MT code.
The anti-pattern implemented in SynchronizableDocument is:

1. Take a lock
2. Change the document
3. Notify a bunch of 3rd party code in listeners about document changes (!!!)
4. Release the lock

The problem here is that listeners can freely acquire any number of locks and trigger any async code during change event notification, while holding the lock on SynchronizableDocument. Taking two or more locks in random order from different threads is the straight road to the deadlock. This is exact the problem we've faced in eclipse-platform/eclipse.platform#282 and I believe I saw this problem with SynchronizableDocument in other places.

A general solution to fix this anti-pattern is to avoid dispatching events while holding a lock.
So the order should be

1. Take a lock
2. Change the document
3. Release the lock
4. Notify a bunch of 3rd party code in listeners about document changes

I have an experimental patch that implements this strategy. Let see how much it will break.
At least for eclipse-platform/eclipse.platform#282 it would unblock the deadlock even without PDE fix eclipse-pde/eclipse.pde#2186 (which is still needed to fix eclipse-pde/eclipse.pde#431).

[laeubi]

I have concerns about the proposed fix. The javadoc states SynchronizableDocument can be "synchronized with a lock object" to allow multiple operations to share synchronization.

More critically, IDocumentListener API specifies:

A listener is informed about document changes before they are applied and after they have been applied

Moving notifications outside the lock would break this contract - another thread could observe an intermediate document state between the change and notification.

The real question: Should listeners be expected to operate lock-free? The current design seems to assume listeners:

• Only perform document operations during notification (which may re-enter)
• Defer any blocking operations to other threads

If listeners are acquiring external locks during notification, that's arguably misuse of the API. Rather than changing the fundamental synchronization model (which could break existing contracts), should we document the expectations for listener implementations?

What do others think about the intended listener contract here?