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

[Alexander Scherbatiy]

On 8/4/2015 12:32 PM, Semyon Sadetsky wrote:
>
>
> On 8/4/2015 11:47 AM, Alexander Scherbatiy wrote:
>> 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.
>>
> Is not it the same as the previous attempt to fix this issue (see 
> 8030118 )?
      8030118 fix does a strange thing like firing InsertUpdate document 
event out of the lock. Do not do that.
> Document change event need to be fired under write lock because the 
> change to the document should be atomic. Queue of changes is undo 
> manager's responsibility not the document.
> And such queue in the AbstractDocument would require complex 
> coordination with all its undo managers queues. What if undo called on 
> undo manager during the doc's queue processing?  The right undo/redo 
> requests and edit events order need to be preserved in this case and 
> it would be too complex or we would have to change the concept and 
> make AbstractDocument to maintain its undo/redo history internally 
> instead of external undo managers.
     It only needs to pass undoable edits in the right order from 
abstract document to the UndoManager.

> Yet another argument do not do this from the user experience: if user 
> starts a long edit operation and press undo after that he expects when 
> the long edit is finished it will be rolled back immediately. 

     It is not true. The first process adds his undo edit to the 
UndoManager. While a user trying to press undo the second long process 
can be started.

> So undo should be executed after the edit is fully performed because 
> the corresponding UndoableEdit which undos this edit can be produced 
> only after the edit is done.
> I think at first we need to look on the situation externally rather 
> than concentrate on implementation questions like in which class do 
> references go.

    Yes, please look on this situation from a user point of view which 
wants to implement simple Java Painter.

    Thanks,
    Alexandr.
>
>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>