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