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

[Semyon Sadetsky]

On 8/4/2015 2:03 PM, Alexander Scherbatiy wrote:
> 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.

Consider the scenario: UndoManager executes undo/redo before it receives 
the undoable edits. As result it will undo not the last edit but 
intermediate and it will crash because the document state is changed and 
intermediate the undoable edit cannot be applied to the final document 
state.

>
>> 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.

That is what led to this issue because when undo is in progress document 
writing should be allowed.
Sorry but I didn't see why is "It not true"? Then what is your 
expectation when you press undo button while edit is not finished yet 
and there is no way to abort it?

>> 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.

But could you describe this scenario? Just steps when this simple 
Painter fails under the proposed fix?I
Note, if this Painter's content is not an AbstarctDocument it will work 
as before the fix.

> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>