<Swing Dev> [9] Review Request for 8030702: Deadlock between subclass of AbstractDocument and UndoManager
Semyon Sadetsky
semyon.sadetsky at oracle.com
Fri Sep 4 18:00:41 UTC 2015
On 9/4/2015 6:11 PM, Alexander Scherbatiy wrote:
> On 9/3/2015 10:01 PM, Semyon Sadetsky wrote:
>>
>>
>> On 8/5/2015 4:50 PM, Semyon Sadetsky wrote:
>>>
>>>
>>> On 8/5/2015 2:39 PM, Alexander Scherbatiy wrote:
>>>> On 8/5/2015 2:00 PM, Semyon Sadetsky wrote:
>>>>>
>>>>>
>>>>> On 8/5/2015 1:04 PM, Alexander Scherbatiy wrote:
>>>>>> On 8/4/2015 8:13 PM, Semyon Sadetsky wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 8/4/2015 6:17 PM, Alexander Scherbatiy wrote:
>>>>>>>> On 8/4/2015 3:13 PM, Semyon Sadetsky wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> This is a good point. But this does not work neither with
>>>>>>>> the current behavior nor with your proposed fix.
>>>>>>>> Consider the following scenario:
>>>>>>>> -----------------------
>>>>>>>> document.insertString("AAA") // "AAA" UndoableEdit is
>>>>>>>> added to the UndoManager
>>>>>>>> document.insertString("BBB")
>>>>>>>> writeLock();
>>>>>>>> handleInsertString();
>>>>>>>> // a user press undo, the "AAA" UndoableEdit is
>>>>>>>> selected in UndoManager but not executed, because of the write
>>>>>>>> lock
>>>>>>>> fireUndoableEditUpdate("BBB") // UndoManager is
>>>>>>>> waiting for the "AAA" UndoableEdit execution
>>>>>>>> writeUnlock() // "AAA" UndoableEdit is executed
>>>>>>>> instead of "BBB"
>>>>>>>> // "BBB" UndoableEdit is
>>>>>>>> added to the UndoManager
>>>>>>>> -----------------------
>>>>>>> It will work after the fix. When undo() method is called it will
>>>>>>> be blocked on the document lock until the edit is done in
>>>>>>> another thread. Then undo() will acquire the document lock and
>>>>>>> call editToBeUndone() method which will return the actual last
>>>>>>> edit added with addEdit() during the undoable callback execution.
>>>>>>
>>>>>> Is it possible to use the same undo manager with several
>>>>>> abstract documents? If so, how are you going to map a document
>>>>>> lock with the document undoable edit without querying it?
>>>>> That scenario is possible. As well as several undo managers can be
>>>>> assigned to the same document. I think I can improve the fix in
>>>>> that direction when you agree with the general approach.
>>>> It is interesting how it is possible to do that without
>>>> querying an undoable edit. Your fix is relaying that an abstract
>>>> document lock should precede the undo manager lock but to get the
>>>> right abstract manager lock you need to take an undoable edit under
>>>> the undo manager lock first.
>>> We always have only two possible directions forward and backward so
>>> it will require only 2 references.
>>
>> Hi Alexander,
>>
>> Please, take a look on the updated version which works for any number
>> documents shared one undo manager.
>> Also I removed the reference you did not like. This has some
>> disadvantages but I think they are negligible.
>> http://cr.openjdk.java.net/~ssadetsky/8030702/webrev.01/
>
> You code looks like:
> ----------------------
> public void undo() throws CannotUndoException {
> synchronized (this) {
> lockedDoc = getLockedDocument(edit);
> }
>
> // TP: 1
>
> while (!done) {
> lockedDoc.writeLock();
> // ...
> lockedDoc.writeUnlock();
> }
> }
> ----------------------
>
> Is it possible that on the line "TP: 1" a new UndoableEdit will be
> added to the UndoManager so the the lockedDoc will not point to the
> latest UndoableEdit which is taken on the line 438.
No. It is not possible because of
438 UndoableEdit edit = editToBeUndone();
It always return the last significant edit so we preserve the consistency.
>
> I still think that updating the UndoableManager for one particular
> AbstarctManager class can be made only after investigation of other
> possibilities.
>
> You could start with choosing behavior which you want to achieve or
> to keep, like:
> - fix the deadlock
> - atomic undo() method
> - serialization
> - immediate roll back action
> - abstract document consistency after undo() action
> - ...
We need to pay attention to the deadlock at first of cause.
Serialization and consistency are achieved. Any concrete doubts?
immediate roll back action -- ?what is that?
I sacrificed undo/redo call atomicity because you did not like doc
references in undo manager. I think it is not important for the most
multithreaded undo/redo scenarios.
>
> and look which of the following approaches can better solve them
> (where the fist is more preferred and the last is less preferred case):
> - using UndoManager as is without adding links from some specific
> classes to it
> - provide an API for UndoManager to work with UndoableEdit-s which
> have synchronization for undo/redo methods
> - adding links of external classes directly to UndoManager
>
What do you mean under link term? A reference or dependency?
We are constrained by compatibility requirements. UndoManager is a
broadly used class we cannot change the API so drastically.
"adding links of external classes directly to UndoManager" - Sorry, did
not catch what are you about? Could you clarify?
--Semyon
> Thanks,
> Alexandr.
>
>>
>> --Semyon
>>
>>>>
>>>>>>
>>>>>>> There is a mistake in your scenario steps:
>>>>>>> fireUndoableEditUpdate() is called before the freeing the lock
>>>>>>> (see AbstractDocument.handleInsertString() method).
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 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?
>>>>>>>> It would be good if it works as you described. But it does
>>>>>>>> not work in this way with or without your fix.
>>>>>>>> undo() action has writeLock in AbstractDocument and because
>>>>>>>> of it is always executed after insert string action.
>>>>>>>> If a user sees that undo is available, he can call it but
>>>>>>>> the second long insertString process can start earlier and
>>>>>>>> acquire the writeLock.
>>>>>>>>
>>>>>>>
>>>>>>> That is what we are going to fix. And this does work after this
>>>>>>> fix. Undo call will be blocked by the long edit until the last
>>>>>>> is done without any deadlocks. And when edit is done undo() will
>>>>>>> acquire the lock and prevent any new edits until undo() is done.
>>>>>>> Please provide a scenario when in your opinion it does not wok.
>>>>>> The first process starts for 5 minutes. When it is finished
>>>>>> a user sees that he can press undo. While he is pressing undo
>>>>>> button, the second long process starts for 10 minutes and acquire
>>>>>> the write lock. The user presses undo but he needs to wait 10
>>>>>> more minutes until the second process is finished.
>>>>> Actually, if two or more threads are waiting for a monitor it is
>>>>> not determined which one will get the control after the signal. To
>>>>> order that the ReentrantLock API could be used but
>>>>> AbstractDocument uses wait/notify for locking. I think it is not
>>>>> worth to dig so deep. It does not cause any issues
>>>> The issue that is considered is "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."
>>>> If you are agree that it is not always possible to do the roll
>>>> back "immediately" there is no point to discussion.
>>> I agree. On that level it is not possible to predict the order
>>> exactly in such scenario. But the state of the document will be
>>> consistent. And it is possible to have it predictable using lock
>>> fairness.
>>>>
>>>>> because undo() always get the last edit anyway. If it will be
>>>>> important for somebody to preserve the execution order on that
>>>>> level of details we will fix 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.
>>>>>>>> Any application that uses UndoManager and wants to have the
>>>>>>>> same synchronization (have the same lock both for UndoableEdit
>>>>>>>> adding and undo() method execution) will have the same deadlock
>>>>>>>> problems.
>>>>>>>> As I have already written:
>>>>>>>> ---------------
>>>>>>>> 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?
>>>>>>>> ---------------
>>>>>>>
>>>>>>> Still do not understand the steps for your Painter scenario. A
>>>>>>> link (reference?) can be added if it is required to implement
>>>>>>> functionality. If the content is not an AbstarctDocument it may
>>>>>>> be required to implement custom UndoManager to support the same
>>>>>>> behavior.
>>>>>>
>>>>>> What is the difference between the AbstractDocument and other
>>>>>> classes (in Swing or user defined)? Do you mean that the
>>>>>> UndoManager is intended only to be used with AbstractDocument and
>>>>>> it shouldn't be used in other cases where undo/redo actions are
>>>>>> required for non text data?
>>>>> No, undo manager can be used with any classes. But since we have
>>>>> it assigned to AbstarctDocument so often we need to do our best to
>>>>> make undo manager working with it correctly because users do not
>>>>> like deadlocks usualy. For other classes we cannot provide
>>>>> synchronization by default because there is no API to get the
>>>>> lock. So it remains up to user how to provide the undo manager
>>>>> synchronization with the object it controls for other classes
>>>> What we should do just to understand that the same deadlock can
>>>> happen in an user applications because he wants to use the same
>>>> synchronization both for the data processing and for the undo
>>>> action. If so, there should be two investigations:
>>>> 1. Is it possible to achieve the requested goals without changing
>>>> UndoManager? In other words The UndoManager should be used in
>>>> proper way as it is required by its design.
>>>> 2. Is it possible to update the UndoManager API to provide
>>>> functionality that meets new requests?
>>> With API change it is reachable. But I would preserve the current
>>> API as less constrained. If we add some methods for locking we will
>>> determine the way how a user should synchronize his undoable
>>> content. And user may not need any synchronization at all. We
>>> should keep in mind this opportunity as well.
>>>>
>>>> Only after this discussion there can be a reason to look to other
>>>> ways.
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>
>>>>> I think our undo manager implementation do not pretend to be used
>>>>> as the global undo manager for big complex applications and it
>>>>> cannot cover all possible undo approaches. But some basic
>>>>> functionality can be provided and it should be usable. Without
>>>>> edits serialization approach it is not usable for multithreaded
>>>>> use. So either we do not pretend to provide a multithreaded undo
>>>>> manager and remove all synchronize keywords from UndoManager
>>>>> class, either we need to support serialization approach which does
>>>>> not cause deadlocks.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>
>>>>>>> I don't see a contradiction here, could you point on it more
>>>>>>> precisely?
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alexandr.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the swing-dev
mailing list