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