<Swing Dev> [9] Review Request for 8030702: Deadlock between subclass of AbstractDocument and UndoManager
Semyon Sadetsky
semyon.sadetsky at oracle.com
Tue Sep 8 09:48:55 UTC 2015
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.
>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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