<Swing Dev> [9] Review Request for 8030702: Deadlock between subclass of AbstractDocument and UndoManager
Semyon Sadetsky
semyon.sadetsky at oracle.com
Thu Sep 3 19:01:22 UTC 2015
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/
--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