<Swing Dev> [9] Review Request for 8030702: Deadlock between subclass of AbstractDocument and UndoManager
Semyon Sadetsky
semyon.sadetsky at oracle.com
Tue Sep 8 11:06:41 UTC 2015
On 9/8/2015 1:07 PM, Alexander Scherbatiy wrote:
> On 9/8/2015 12:48 PM, Semyon Sadetsky wrote:
>>
>>
>> On 9/8/2015 12:26 PM, Alexander Scherbatiy wrote:
>>> On 9/8/2015 11:28 AM, Semyon Sadetsky wrote:
>>>>
>>>>
>>>> On 9/7/2015 5:56 PM, Alexander Scherbatiy wrote:
>>>>> On 9/7/2015 5:08 PM, Semyon Sadetsky wrote:
>>>>>>
>>>>>>
>>>>>> On 9/7/2015 2:41 PM, Alexander Scherbatiy wrote:
>>>>>>> On 9/4/2015 9:00 PM, Semyon Sadetsky wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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 see.
>>>>>>>
>>>>>>> There is one more question about the undoOrRedo() method
>>>>>>> there the synchronization is removed.
>>>>>>> Lets look at the sequences of calls to an UndoManager:
>>>>>>> addEdit(anEdit1), addEdit(anEdit2), undoOrRedo(), undoOrRedo().
>>>>>>>
>>>>>>> The result for two undoOrRedo() calls should neutralize each
>>>>>>> other (it is just undo and redo calls).
>>>>>>>
>>>>>>> Is it possible that after the fix the the first undoOrRedo()
>>>>>>> from one thread fails to do the redo and before starting to do
>>>>>>> the undo
>>>>>>> the second undoOrRedo() call from another thread also fails to
>>>>>>> do a redo action?
>>>>>>> In this cases two undo actions could be called instead of undo
>>>>>>> and redo.
>>>>>>
>>>>>> It does not make any sense to make atomic the convenience method
>>>>>> undoOrRedo() because undo() and redo() are not atomic.
>>>>>
>>>>> All UndoManager.undo()/redo()/undoOrRedo() have synchronization.
>>>>>
>>>>> For the sample described above two undoOrRedo() calls always
>>>>> invoke undo() at first and redo() at the second step.
>>>>> After the fix it is possible that two undo() methods can be
>>>>> called. It means that there will be a regression in the
>>>>> undoOrRedo() method behavior after the fix.
>>>> The spec of the method to not promise any deterministic behavior
>>>> for two consequent calls.
>>>
>>> javadoc for UndoManager.undoOrRedo() method [1]:
>>> "Convenience method that invokes one of undo or redo. If any edits
>>> have been undone (the index of the next edit is less than the length
>>> of the edits list) this invokes redo, otherwise it invokes undo."
>>>
>>> For the sequence of calls addEdit(anEdit1), addEdit(anEdit2),
>>> undoOrRedo(), undoOrRedo():
>>> Step 1: call undoOrRedo() - there are no undone edits, the undo
>>> should be invoked.
>>> Step 2: call undoOrRedo() - there is one undone edit, the redo
>>> should be invoked.
>>>
>>>
>>> [1]
>>> http://docs.oracle.com/javase/8/docs/api/javax/swing/undo/UndoManager.html#undoOrRedo--
>>>
>> I think it is obvious that this never worked if addEdit() and
>> undoOrRedo() are called from different threads without the external
>> synchronization.
>> The fix changes nothing here. It works fine in a single thread approach.
>
> The javadoc does not mention that it is not thread safe to use the
> undoOrRedo() method from different treads.
> You can get all permutations of the "addEdit(anEdit1),
> addEdit(anEdit2), undoOrRedo(), undoOrRedo()" calls and check the the
> fix does not break cases which worked before the fix.
>
A sequence of calls can be permuted in a single thread? How?
> Thanks,
> Alexandr.
It is
>
>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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?
>>>>>>> "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." - what ever does it mean.
>>>>>> Got it. It will work within the fairness. We have discussed this
>>>>>> allready.
>>>>>>>> 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.
>>>>>>> Could you give more details about it. Which doc references do
>>>>>>> you mean?
>>>>>>
>>>>>> Your statement a dozen iterations ago was: "There should be a
>>>>>> way to solve these problems without storing links of external
>>>>>> classes inside the UndoManager."
>>>>>> I guess you used "link" term for references. I would recommend to
>>>>>> use standard terminology: reference to the object, dependency on
>>>>>> the class, etc... to avoid misunderstanding.
>>>>>> Usually "link" is in a browser document or a tool that produces
>>>>>> executables after compilation.
>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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?
>>>>>>> There are two options. If UndoManager is a class designed to
>>>>>>> be only used with the AbstractDocument and placed in the
>>>>>>> javax.swing.text package it definitly can have special code to
>>>>>>> handle an abstract document instance in a special way.
>>>>>>> If UndoManager is a general purpose class, it looks strange
>>>>>>> that it handles some special classes in different way as all
>>>>>>> others. It usually mean that there are some design problems in
>>>>>>> this class. That is why I just asked to look at other ways at
>>>>>>> first. Only if other solutions are not suitable it has sense to
>>>>>>> look at the way that you are provided.
>>>>>>>
>>>>>> Correct. I introduced extra dependency. It is optional, but
>>>>>> anyway. Of cause there is a design problem in undo
>>>>>> javax.swing.undo package. But I cannot rewrite the API because we
>>>>>> will get a compatibility problem then. I mentioned this several
>>>>>> times in this thread.
>>>>>>
>>>>>>>
>>>>>>>> We are constrained by compatibility requirements. UndoManager
>>>>>>>> is a broadly used class we cannot change the API so drastically.
>>>>>>>
>>>>>>> I think that you can generalize your solution just adding an
>>>>>>> internal interface like sun.swing.UndoableEditLock.
>>>>>>> Every UndoableEdit which implements this interface can
>>>>>>> provide a lock for its synchronization.
>>>>>>>
>>>>>>> If this will work it can be made public in some days so other
>>>>>>> application can also have proper synchronization for their
>>>>>>> undo/redo actions.
>>>>>> OK. I added it.
>>>>>> http://cr.openjdk.java.net/~ssadetsky/8030702/webrev.02/
>>>>>
>>>>> - We can return a public class that implements an internal
>>>>> interface, but we can't expose an internal API in the public class
>>>>> definition.
>>>>> May be it is possible to wrap an UndoableEdit to the
>>>>> UndoableEditLockSupport in the UndoableEditEvent or in some other
>>>>> place.
>>>>>
>>>>> - The similar code is used both in the UndoManager.undo() and
>>>>> redo() methods. Is it possible to move this code to one method
>>>>> that does undo or redo depending on the given argument?
>>>>>
>>>> OK. accepted.
>>>> http://cr.openjdk.java.net/~ssadetsky/8030702/webrev.03/
>>>
>>> - UndoManager.undo/redo methods
>>> In your previous fix inProgress variable and the super call
>>> were used under the lock. It may leads to some synchronization
>>> issues if you decide to omit it.
>>> - UndoManager.tryUndoOrRedo()
>>> It is possible to get rid of 'done' variable just using an
>>> infinity loop and return the exact result where the loop is terminated.
>>> - AbstractDocument.DefaultDocumentEventUndoableWrapper implements
>>> both UndoableEdit and UndoableEditLockSupport interfaces but
>>> UndoableEditLockSupport already extends UndoableEdit.
>>> - "@since 1.9" javadoc for
>>> DefaultDocumentEventUndoableWrapper.lockEdit()/unlockEdit() methods
>>> really belongs to the UndoableEditLockSupport methods.
>>> In this case there is no need for {@inheritDoc} tag.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>>>
>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>>> "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