<Swing Dev> [9] Review Request for 8030702: Deadlock between subclass of AbstractDocument and UndoManager

[Alexander Scherbatiy]

On 8/3/2015 4:19 PM, Semyon Sadetsky wrote:
> On 8/3/2015 3:12 PM, Alexander Scherbatiy wrote:
>> On 7/31/2015 9:44 AM, Semyon Sadetsky wrote:
>>> Good question. But I did not add a concrete class.
>>> The problem is that UndoManager provided by JDK wants to be 
>>> serialized but undoable objects knows nothing about that. The 
>>> contract between UndoManager and undoable is UndoableEditListener 
>>> which only notifies UndoManager to add a new edit. AbstractDocument 
>>> does not care about the specific UndoManager implementation and it 
>>> can contain plenty different UndoableEditListener. That is the 
>>> current API approach.
>>> If our specific UndoManager wants to be serialized it should also 
>>> take into account that the undoable it controls may require 
>>> serialization. For that it needs undoable's synchronization monitor 
>>> and AbstractDocument can provide it using writeLock()/writeUnlock() 
>>> methods. I assumed that in the first turn UndoManger should work 
>>> well with JDK undoables than to serve as a general implementation. 
>>> Also I tried to preserve  the current API.
>>> And your suggestion is to change the existing UndoableEditListener 
>>> API by introducing synchronization methods in it. Am I correctly 
>>> understand you?
>>
>>    What I said is that UndoManager can be used not only by 
>> AbstractDocument but also in other classes which can have the same 
>> synchronization problems.
>>    There should be a way to solve these problems without storing 
>> links of external classes inside the UndoManager.
>>
>
> As well as AbstractDocument can use another undo managers. It can be 
> addressed to both parties. They need each others locks to serialize 
> changes without deadlock.
> AbstarctDocument is related to UndoableEditListener as one to many 
> that means a lock should be taken for each undo manager before the 
> document change.
> Undo manager does not have any methods to get its lock because it is 
> an UndoableEditListener implementation. AbstarctDocument has API to 
> receive its lock.
> Do you still propose to fix the issue on AbstractDocument side? 
    Yes.
> Could you clarify how do you see such fix?

     Put an UndoableEdit/UndoableEditEvent/necessary information to a 
queue instead of firing the undoable edit event under the write lock. Do 
not read the queue under the write lock. The queue allows to preserve 
the order of UndoableEdit's adding to an UndoManager.

   Thanks,
   Alexandr.

>
> --Semyon
>
>> Thanks,
>>   Alexandr.
>>
>>>
>>> --Semyon
>>>
>>>
>>> On 7/30/2015 5:27 PM, Alexander Scherbatiy wrote:
>>>>
>>>>    Consider someone writes Java Painter application where it is 
>>>> possible to draw lines and images and uses UndoManager for 
>>>> undo/redo actions.
>>>>    He might want that it was possible to work with copied images. 
>>>> He can get lock on ctrl+v action, process an image, prepare 
>>>> UndoableEdit and notify the UndoManager.
>>>>    He also can use lock/unlock in the undo action to have a 
>>>> consistent state with the processed image. If someone calls undo 
>>>> action during the image processing and gets a deadlock does it mean 
>>>> that link from Java Painter need to be added to the UndoManager?
>>>>
>>>>   Thanks,
>>>>   Alexandr.
>>>>
>>>>>>    It looks like AbstractDocument violates UndoManager 
>>>>>> synchronization contract when it both use lock to work with 
>>>>>> UndoManager and in the implemented undo() method.
>>>>>>
>>>>>>   Thanks,
>>>>>>   Alexandr.
>>>>>>
>>>>>>>
>>>>>>> --Semyon
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>