<Swing Dev> [9] Review Request for 8030702: Deadlock between subclass of AbstractDocument and UndoManager
Semyon Sadetsky
semyon.sadetsky at oracle.com
Fri Sep 11 16:24:16 UTC 2015
On 9/11/2015 6:04 PM, Alexander Scherbatiy wrote:
> On 9/10/2015 7:30 PM, Semyon Sadetsky wrote:
>>
>>
>> On 9/10/2015 6:46 PM, Alexander Scherbatiy wrote:
>>> On 9/10/2015 6:26 PM, Semyon Sadetsky wrote:
>>>>
>>>>
>>>> On 9/10/2015 5:13 PM, Alexander Scherbatiy wrote:
>>>>> On 9/10/2015 5:07 PM, Semyon Sadetsky wrote:
>>>>>>
>>>>>>
>>>>>> On 9/10/2015 3:45 PM, Alexander Scherbatiy wrote:
>>>>>>> On 9/10/2015 3:39 PM, Semyon Sadetsky wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/10/2015 2:44 PM, Alexander Scherbatiy wrote:
>>>>>>>>> On 9/8/2015 6:11 PM, Semyon Sadetsky wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 9/8/2015 2:10 PM, Alexander Scherbatiy wrote:
>>>>>>>>>>> On 9/8/2015 2:06 PM, Semyon Sadetsky wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> 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?
>>>>>>>>>>>
>>>>>>>>>>> Why are you asking about a single thread? The UndoManager
>>>>>>>>>>> class is designed to work with multiple threads.
>>>>>>>>>>>
>>>>>>>>>>> You need to look at the all possible cases which can be
>>>>>>>>>>> called by an application from different threads like:
>>>>>>>>>>> - addEdit(anEdit1), addEdit(anEdit2), undoOrRedo(),
>>>>>>>>>>> undoOrRedo()
>>>>>>>>>>> - addEdit(anEdit1), undoOrRedo(), addEdit(anEdit2),
>>>>>>>>>>> undoOrRedo()
>>>>>>>>>>> - ...
>>>>>>>>>>>
>>>>>>>>>>> and check that there are no regressions in the behavior
>>>>>>>>>>> after the fix.
>>>>>>>>>> Are you kidding? This UndoManager sample has never worked in
>>>>>>>>>> multithreading because of deadlock issue we are fixing.
>>>>>>>>>> What the regression are you talking about?
>>>>>>>>>
>>>>>>>>> I am sorry. I do not understand your sense of humor.
>>>>>>>>>
>>>>>>>>> Do you mean that the deadlock happens every time when an
>>>>>>>>> UndoEdiale is added to a UndoManager and undo() is called and
>>>>>>>>> because of this the the UndoManager is totally unusable?
>>>>>>>> In you suggestion you want to run addEdit(anEdit1),
>>>>>>>> addEdit(anEdit2), undoOrRedo(), undoOrRedo() not in single
>>>>>>>> thread but from different threads two times: before the fix and
>>>>>>>> after it then to compare the results. But it is just impossible
>>>>>>>> to get any valuable results from the run without the fix
>>>>>>>> because it will crash with the deadlock. The deadlock we are
>>>>>>>> trying to fix.
>>>>>>>> Do you see why it is not possible or you need extra explanation?
>>>>>>>
>>>>>>> Even though something does not work before the fix the
>>>>>>> UndoManager has been designed to work in the multi-tread
>>>>>>> environment and the undoOrRedo() method should work in the
>>>>>>> predicted way.
>>>>>> The sample is related to singlethreaded usage of cause.
>>>>>>>
>>>>>>> If you think that it can't be fixed with this fix you can file
>>>>>>> a new issue that it is a problem which we aware of.
>>>>>> We discussed that already. The order is not deterministic without
>>>>>> the external fair lock. And I think it is not needed for 99.(9)%
>>>>>> of usages. Nobody cares if an undo button pressed 1 ms early or 1
>>>>>> ms later. It shouldn't be critical otherwise it is anti-pattern.
>>>>>> If the background editing is allowed concurrently along the user
>>>>>> interaction one need to use an external synchronization or simply
>>>>>> disable undo/redo until the editing is done.
>>>>>> For me it absolutely clear that it is a degenerated scenario when
>>>>>> a UI document is modified concurrently from two different
>>>>>> threads, but we should not allow deadlocks anyway and have to fix
>>>>>> it.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>
>>>>>>>
>>>>>>>>> Do you think that it is a good idea to remove a
>>>>>>>>> synchronization from the undoOrRedo method because there is
>>>>>>>>> the deadlock?
>>>>>>>>> If so, may be you can just remove the synchronization
>>>>>>>>> keyword form the undo() and redo() methods as well and the
>>>>>>>>> deadlock just goes away?
>>>>>>>>> No new interfaces, no locks from an abstract document. That
>>>>>>>>> would be the simplest solution of the issue.
>>>>>
>>>>> Could you answer on the question why do you suggest so
>>>>> complicated fix? Just remove the synchronization from the undo()
>>>>> and redo() methods and you really fix the deadlock.
>>>>> It seems that you believe that it is possible because: "If the
>>>>> background editing is allowed concurrently along the user
>>>>> interaction one need to use an external synchronization or simply
>>>>> disable undo/redo until the editing is done."
>>>> Strange suggestion. We cannot remove synchronization it is required
>>>> to guarantee consistency.
>>>
>>> The same is for the undoOrRedo() method. You need to provide a
>>> synchronization for it or create an new issue on it.
>> undoOrRedo() just calls undo() or redo() which contain all necessary
>> synchronization.
>
> UndoManager is designed as a general purpose class which can be
> used from multi-thread environment even for applications which do not
> use abstract documents.
> Consider such application which creates its UndoableEdit events
> where it does not use locks in undo() and redo() methods. This
> application can use UndoManager.undo()/redo()/undoOrRedo() methods
> from different threads without deadlock.
> This application relies on the UndoManager.undoOrRedo()
> description that the right action always will be called.
> After the fix it may happen that two UndoManager.undoOrRedo()
> calls can leads to two undo() calls instead of undo() and redo(). This
> is the regression from the previous behavior.
I showed you already that the previous behavior was a deadlock for this
scenario. No regression here.
>
> Thanks,
> Alexandr.
>
>>>
>>> Thanks,
>>> Alexandr.
>>>> Also synchronization is needed to provide coherency with the state
>>>> of the document. If those two are not provided the document can be
>>>> corrupted. This is enough most usages.
>>>>>
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alexandr.
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Alexandr.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> 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