<Swing Dev> [9] Review Request for 8030702: Deadlock between subclass of AbstractDocument and UndoManager
Semyon Sadetsky
semyon.sadetsky at oracle.com
Wed Sep 16 14:14:14 UTC 2015
On 9/16/2015 1:38 PM, Alexander Scherbatiy wrote:
> On 9/15/2015 10:37 PM, Semyon Sadetsky wrote:
>>
>>
>> On 9/15/2015 6:16 PM, Alexander Scherbatiy wrote:
>>> On 9/11/2015 7:24 PM, Semyon Sadetsky wrote:
>>>>
>>>
>>> The deadlock that you described exists only because
>>> AbstractDocument uses locks in the UndoableEdit.undo()/redo() methods.
>>> Applications that use UndoManager and do not use lock in the
>>> UndoableEdit.undo()/redo() methods do not have deadlock. They worked
>>> fine before the fix and can lost data consistency after the fix.
>>> This is definitely the regression.
>> You mean scenario when a document that does not support
>> synchronization but anyway is modified from several threads.
>> You can expect that such scenario is functional only if such document
>> is a single atomic field.
>> I updated the fix
>> http://cr.openjdk.java.net/~ssadetsky/8030702/webrev.04/ take it into
>> account.
>
> This looks better. There are just some comments:
> - The 'inProgress' variable in
> UndoManager.undo()/redo()/undoOrRedo() methods should have
> synchronization.
> Is it possible to move 'if (inProgress)' check into
> tryUndoOrRedo() method similarly to as it was used in the version 2 of
> the fix?
> - UndoManager line 489: why not to use the original check from the
> undoOrRedo() method "if (indexOfNextAdd == edits.size())" to pick up
> undo or redo action?
> - UndoManager line 516: An undoable edit can be requested two times
> for the ANY action because the 'undo' variable can have old value in
> the second synchronized block.
> Even the logic is right it is better to take an edit based on the
> 'action' variable.
> - UndoManager.undoOrRedo() throws CannotUndoException now instead
> of CannotRedoException if the redo action is not possible.
> - It is possible to get rid of the 'done ' variable in
> UndoManager.tryUndoOrRedo() just simply have an infinity loop.
> - It is possible to use Boolean values TRUE, FALSE or null for
> three-state logic. But it is up to you to choose enums or Boolean in
> the fix.
>
I made the changes:
http://cr.openjdk.java.net/~ssadetsky/8030702/webrev.05/ except for the
last one. Actually triple Boolean logic is a bad style.
> Thanks,
> Alexandr.
>>>
>>> Thanks,
>>> Alexandr.
>>>>> Thanks,
>>>>> Alexandr.
>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Alexandr.
>>>>>>>> Also synchronization is needed to provide coherency with the
>>>>>>>> state of the document. If those two are not provided the
>>>>>>>> document can be corrupted. This is enough most usages.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alexandr.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>> It is
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> I still think that updating the
>>>>>>>>>>>>>>>>>>>>>>>>> UndoableManager for one particular
>>>>>>>>>>>>>>>>>>>>>>>>> AbstarctManager class can be made only after
>>>>>>>>>>>>>>>>>>>>>>>>> investigation of other possibilities.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> You could start with choosing behavior
>>>>>>>>>>>>>>>>>>>>>>>>> which you want to achieve or to keep, like:
>>>>>>>>>>>>>>>>>>>>>>>>> - fix the deadlock
>>>>>>>>>>>>>>>>>>>>>>>>> - atomic undo() method
>>>>>>>>>>>>>>>>>>>>>>>>> - serialization
>>>>>>>>>>>>>>>>>>>>>>>>> - immediate roll back action
>>>>>>>>>>>>>>>>>>>>>>>>> - abstract document consistency after
>>>>>>>>>>>>>>>>>>>>>>>>> undo() action
>>>>>>>>>>>>>>>>>>>>>>>>> - ...
>>>>>>>>>>>>>>>>>>>>>>>> We need to pay attention to the deadlock at
>>>>>>>>>>>>>>>>>>>>>>>> first of cause.
>>>>>>>>>>>>>>>>>>>>>>>> Serialization and consistency are achieved. Any
>>>>>>>>>>>>>>>>>>>>>>>> concrete doubts?
>>>>>>>>>>>>>>>>>>>>>>>> immediate roll back action -- ?what is that?
>>>>>>>>>>>>>>>>>>>>>>> "if user starts a long edit operation and
>>>>>>>>>>>>>>>>>>>>>>> press undo after that he expects when the long
>>>>>>>>>>>>>>>>>>>>>>> edit is finished it will be rolled back
>>>>>>>>>>>>>>>>>>>>>>> immediately." - what ever does it mean.
>>>>>>>>>>>>>>>>>>>>>> Got it. It will work within the fairness. We have
>>>>>>>>>>>>>>>>>>>>>> discussed this allready.
>>>>>>>>>>>>>>>>>>>>>>>> I sacrificed undo/redo call atomicity because
>>>>>>>>>>>>>>>>>>>>>>>> you did not like doc references in undo
>>>>>>>>>>>>>>>>>>>>>>>> manager. I think it is not important for the
>>>>>>>>>>>>>>>>>>>>>>>> most multithreaded undo/redo scenarios.
>>>>>>>>>>>>>>>>>>>>>>> Could you give more details about it. Which
>>>>>>>>>>>>>>>>>>>>>>> doc references do you mean?
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Your statement a dozen iterations ago was: "There
>>>>>>>>>>>>>>>>>>>>>> should be a way to solve these problems without
>>>>>>>>>>>>>>>>>>>>>> storing links of external classes inside the
>>>>>>>>>>>>>>>>>>>>>> UndoManager."
>>>>>>>>>>>>>>>>>>>>>> I guess you used "link" term for references. I
>>>>>>>>>>>>>>>>>>>>>> would recommend to use standard terminology:
>>>>>>>>>>>>>>>>>>>>>> reference to the object, dependency on the class,
>>>>>>>>>>>>>>>>>>>>>> etc... to avoid misunderstanding.
>>>>>>>>>>>>>>>>>>>>>> Usually "link" is in a browser document or a tool
>>>>>>>>>>>>>>>>>>>>>> that produces executables after compilation.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> and look which of the following approaches
>>>>>>>>>>>>>>>>>>>>>>>>> can better solve them (where the fist is more
>>>>>>>>>>>>>>>>>>>>>>>>> preferred and the last is less preferred case):
>>>>>>>>>>>>>>>>>>>>>>>>> - using UndoManager as is without adding
>>>>>>>>>>>>>>>>>>>>>>>>> links from some specific classes to it
>>>>>>>>>>>>>>>>>>>>>>>>> - provide an API for UndoManager to work
>>>>>>>>>>>>>>>>>>>>>>>>> with UndoableEdit-s which have synchronization
>>>>>>>>>>>>>>>>>>>>>>>>> for undo/redo methods
>>>>>>>>>>>>>>>>>>>>>>>>> - adding links of external classes directly
>>>>>>>>>>>>>>>>>>>>>>>>> to UndoManager
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> What do you mean under link term? A reference
>>>>>>>>>>>>>>>>>>>>>>>> or dependency?
>>>>>>>>>>>>>>>>>>>>>>> There are two options. If UndoManager is a
>>>>>>>>>>>>>>>>>>>>>>> class designed to be only used with the
>>>>>>>>>>>>>>>>>>>>>>> AbstractDocument and placed in the
>>>>>>>>>>>>>>>>>>>>>>> javax.swing.text package it definitly can have
>>>>>>>>>>>>>>>>>>>>>>> special code to handle an abstract document
>>>>>>>>>>>>>>>>>>>>>>> instance in a special way.
>>>>>>>>>>>>>>>>>>>>>>> If UndoManager is a general purpose class,
>>>>>>>>>>>>>>>>>>>>>>> it looks strange that it handles some special
>>>>>>>>>>>>>>>>>>>>>>> classes in different way as all others. It
>>>>>>>>>>>>>>>>>>>>>>> usually mean that there are some design problems
>>>>>>>>>>>>>>>>>>>>>>> in this class. That is why I just asked to look
>>>>>>>>>>>>>>>>>>>>>>> at other ways at first. Only if other solutions
>>>>>>>>>>>>>>>>>>>>>>> are not suitable it has sense to look at the way
>>>>>>>>>>>>>>>>>>>>>>> that you are provided.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Correct. I introduced extra dependency. It is
>>>>>>>>>>>>>>>>>>>>>> optional, but anyway. Of cause there is a design
>>>>>>>>>>>>>>>>>>>>>> problem in undo javax.swing.undo package. But I
>>>>>>>>>>>>>>>>>>>>>> cannot rewrite the API because we will get a
>>>>>>>>>>>>>>>>>>>>>> compatibility problem then. I mentioned this
>>>>>>>>>>>>>>>>>>>>>> several times in this thread.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> We are constrained by compatibility
>>>>>>>>>>>>>>>>>>>>>>>> requirements. UndoManager is a broadly used
>>>>>>>>>>>>>>>>>>>>>>>> class we cannot change the API so drastically.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> I think that you can generalize your solution
>>>>>>>>>>>>>>>>>>>>>>> just adding an internal interface like
>>>>>>>>>>>>>>>>>>>>>>> sun.swing.UndoableEditLock.
>>>>>>>>>>>>>>>>>>>>>>> Every UndoableEdit which implements this
>>>>>>>>>>>>>>>>>>>>>>> interface can provide a lock for its
>>>>>>>>>>>>>>>>>>>>>>> synchronization.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> If this will work it can be made public in
>>>>>>>>>>>>>>>>>>>>>>> some days so other application can also have
>>>>>>>>>>>>>>>>>>>>>>> proper synchronization for their undo/redo actions.
>>>>>>>>>>>>>>>>>>>>>> OK. I added it.
>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8030702/webrev.02/
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> - We can return a public class that implements
>>>>>>>>>>>>>>>>>>>>> an internal interface, but we can't expose an
>>>>>>>>>>>>>>>>>>>>> internal API in the public class definition.
>>>>>>>>>>>>>>>>>>>>> May be it is possible to wrap an UndoableEdit
>>>>>>>>>>>>>>>>>>>>> to the UndoableEditLockSupport in the
>>>>>>>>>>>>>>>>>>>>> UndoableEditEvent or in some other place.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> - The similar code is used both in the
>>>>>>>>>>>>>>>>>>>>> UndoManager.undo() and redo() methods. Is it
>>>>>>>>>>>>>>>>>>>>> possible to move this code to one method that does
>>>>>>>>>>>>>>>>>>>>> undo or redo depending on the given argument?
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> OK. accepted.
>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8030702/webrev.03/
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> - UndoManager.undo/redo methods
>>>>>>>>>>>>>>>>>>> In your previous fix inProgress variable and
>>>>>>>>>>>>>>>>>>> the super call were used under the lock. It may
>>>>>>>>>>>>>>>>>>> leads to some synchronization issues if you decide
>>>>>>>>>>>>>>>>>>> to omit it.
>>>>>>>>>>>>>>>>>>> - UndoManager.tryUndoOrRedo()
>>>>>>>>>>>>>>>>>>> It is possible to get rid of 'done' variable
>>>>>>>>>>>>>>>>>>> just using an infinity loop and return the exact
>>>>>>>>>>>>>>>>>>> result where the loop is terminated.
>>>>>>>>>>>>>>>>>>> -
>>>>>>>>>>>>>>>>>>> AbstractDocument.DefaultDocumentEventUndoableWrapper
>>>>>>>>>>>>>>>>>>> implements both UndoableEdit and
>>>>>>>>>>>>>>>>>>> UndoableEditLockSupport interfaces but
>>>>>>>>>>>>>>>>>>> UndoableEditLockSupport already extends UndoableEdit.
>>>>>>>>>>>>>>>>>>> - "@since 1.9" javadoc for
>>>>>>>>>>>>>>>>>>> DefaultDocumentEventUndoableWrapper.lockEdit()/unlockEdit()
>>>>>>>>>>>>>>>>>>> methods really belongs to the
>>>>>>>>>>>>>>>>>>> UndoableEditLockSupport methods.
>>>>>>>>>>>>>>>>>>> In this case there is no need for {@inheritDoc}
>>>>>>>>>>>>>>>>>>> tag.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> "adding links of external classes directly to
>>>>>>>>>>>>>>>>>>>>>>>> UndoManager" - Sorry, did not catch what are
>>>>>>>>>>>>>>>>>>>>>>>> you about? Could you clarify?
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> There is a mistake in your scenario
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> steps: fireUndoableEditUpdate() is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> called before the freeing the lock (see
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> AbstractDocument.handleInsertString()
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> method).
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yet another argument do not do this
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> from the user experience: if user
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> starts a long edit operation and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> press undo after that he expects
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> when the long edit is finished it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> will be rolled back immediately.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> It is not true. The first process
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> adds his undo edit to the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> UndoManager. While a user trying to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> press undo the second long process
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> can be started.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> That is what led to this issue because
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> when undo is in progress document
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> writing should be allowed.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Sorry but I didn't see why is "It not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> true"? Then what is your expectation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> when you press undo button while edit
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is not finished yet and there is no
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> way to abort it?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> It would be good if it works as you
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> described. But it does not work in this
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> way with or without your fix.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> undo() action has writeLock in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> AbstractDocument and because of it is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> always executed after insert string
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> action.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> If a user sees that undo is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> available, he can call it but the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> second long insertString process can
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> start earlier and acquire the writeLock.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> That is what we are going to fix. And
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> this does work after this fix. Undo call
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> will be blocked by the long edit until
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the last is done without any deadlocks.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> And when edit is done undo() will
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> acquire the lock and prevent any new
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> edits until undo() is done. Please
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> provide a scenario when in your opinion
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it does not wok.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The first process starts for 5
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> minutes. When it is finished a user sees
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> that he can press undo. While he is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> pressing undo button, the second long
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> process starts for 10 minutes and acquire
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the write lock. The user presses undo but
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> he needs to wait 10 more minutes until
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the second process is finished.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Actually, if two or more threads are
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> waiting for a monitor it is not determined
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which one will get the control after the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> signal. To order that the ReentrantLock
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> API could be used but AbstractDocument
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> uses wait/notify for locking. I think it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> is not worth to dig so deep. It does not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> cause any issues
>>>>>>>>>>>>>>>>>>>>>>>>>>>> The issue that is considered is "if
>>>>>>>>>>>>>>>>>>>>>>>>>>>> user starts a long edit operation and press
>>>>>>>>>>>>>>>>>>>>>>>>>>>> undo after that he expects when the long
>>>>>>>>>>>>>>>>>>>>>>>>>>>> edit is finished it will be rolled back
>>>>>>>>>>>>>>>>>>>>>>>>>>>> immediately."
>>>>>>>>>>>>>>>>>>>>>>>>>>>> If you are agree that it is not always
>>>>>>>>>>>>>>>>>>>>>>>>>>>> possible to do the roll back "immediately"
>>>>>>>>>>>>>>>>>>>>>>>>>>>> there is no point to discussion.
>>>>>>>>>>>>>>>>>>>>>>>>>>> I agree. On that level it is not possible to
>>>>>>>>>>>>>>>>>>>>>>>>>>> predict the order exactly in such scenario.
>>>>>>>>>>>>>>>>>>>>>>>>>>> But the state of the document will be
>>>>>>>>>>>>>>>>>>>>>>>>>>> consistent. And it is possible to have it
>>>>>>>>>>>>>>>>>>>>>>>>>>> predictable using lock fairness.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> because undo() always get the last edit
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> anyway. If it will be important for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> somebody to preserve the execution order
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> on that level of details we will fix it.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> So undo should be executed after the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> edit is fully performed because the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> corresponding UndoableEdit which
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> undos this edit can be produced only
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> after the edit is done.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I think at first we need to look on
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the situation externally rather than
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> concentrate on implementation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> questions like in which class do
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> references go.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Yes, please look on this situation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> from a user point of view which wants
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to implement simple Java Painter.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> But could you describe this scenario?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Just steps when this simple Painter
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> fails under the proposed fix?I
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Note, if this Painter's content is not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> an AbstarctDocument it will work as
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> before the fix.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Any application that uses UndoManager
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and wants to have the same
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> synchronization (have the same lock
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> both for UndoableEdit adding and undo()
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> method execution) will have the same
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> deadlock problems.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> As I have already written:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---------------
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Consider someone writes Java Painter
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> application where it is possible to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> draw lines and images and uses
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> UndoManager for undo/redo actions.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> He might want that it was possible
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to work with copied images. He can get
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lock on ctrl+v action, process an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> image, prepare UndoableEdit and notify
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the UndoManager.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> He also can use lock/unlock in the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> undo action to have a consistent state
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> with the processed image. If someone
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> calls undo action during the image
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> processing and gets a deadlock does it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> mean that link from Java Painter need
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> to be added to the UndoManager?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ---------------
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Still do not understand the steps for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> your Painter scenario. A link
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (reference?) can be added if it is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> required to implement functionality. If
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the content is not an AbstarctDocument
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it may be required to implement custom
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> UndoManager to support the same behavior.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> What is the difference between the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> AbstractDocument and other classes (in
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Swing or user defined)? Do you mean that
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the UndoManager is intended only to be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> used with AbstractDocument and it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> shouldn't be used in other cases where
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> undo/redo actions are required for non
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> text data?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> No, undo manager can be used with any
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> classes. But since we have it assigned to
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> AbstarctDocument so often we need to do
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> our best to make undo manager working with
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it correctly because users do not like
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> deadlocks usualy. For other classes we
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> cannot provide synchronization by default
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> because there is no API to get the lock.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> So it remains up to user how to provide
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the undo manager synchronization with the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> object it controls for other classes
>>>>>>>>>>>>>>>>>>>>>>>>>>>> What we should do just to understand
>>>>>>>>>>>>>>>>>>>>>>>>>>>> that the same deadlock can happen in an
>>>>>>>>>>>>>>>>>>>>>>>>>>>> user applications because he wants to use
>>>>>>>>>>>>>>>>>>>>>>>>>>>> the same synchronization both for the data
>>>>>>>>>>>>>>>>>>>>>>>>>>>> processing and for the undo action. If so,
>>>>>>>>>>>>>>>>>>>>>>>>>>>> there should be two investigations:
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 1. Is it possible to achieve the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> requested goals without changing
>>>>>>>>>>>>>>>>>>>>>>>>>>>> UndoManager? In other words The UndoManager
>>>>>>>>>>>>>>>>>>>>>>>>>>>> should be used in proper way as it is
>>>>>>>>>>>>>>>>>>>>>>>>>>>> required by its design.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 2. Is it possible to update the
>>>>>>>>>>>>>>>>>>>>>>>>>>>> UndoManager API to provide functionality
>>>>>>>>>>>>>>>>>>>>>>>>>>>> that meets new requests?
>>>>>>>>>>>>>>>>>>>>>>>>>>> With API change it is reachable. But I would
>>>>>>>>>>>>>>>>>>>>>>>>>>> preserve the current API as less
>>>>>>>>>>>>>>>>>>>>>>>>>>> constrained. If we add some methods for
>>>>>>>>>>>>>>>>>>>>>>>>>>> locking we will determine the way how a user
>>>>>>>>>>>>>>>>>>>>>>>>>>> should synchronize his undoable content. And
>>>>>>>>>>>>>>>>>>>>>>>>>>> user may not need any synchronization at
>>>>>>>>>>>>>>>>>>>>>>>>>>> all. We should keep in mind this opportunity
>>>>>>>>>>>>>>>>>>>>>>>>>>> as well.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Only after this discussion there can be a
>>>>>>>>>>>>>>>>>>>>>>>>>>>> reason to look to other ways.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I think our undo manager implementation do
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> not pretend to be used as the global undo
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> manager for big complex applications and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it cannot cover all possible undo
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> approaches. But some basic functionality
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> can be provided and it should be usable.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Without edits serialization approach it is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> not usable for multithreaded use. So
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> either we do not pretend to provide a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> multithreaded undo manager and remove all
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> synchronize keywords from UndoManager
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> class, either we need to support
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> serialization approach which does not
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> cause deadlocks.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I don't see a contradiction here, could
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> you point on it more precisely?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> On 7/30/2015 5:27 PM, Alexander
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Scherbatiy wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Consider someone writes Java
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Painter application where it is
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> possible to draw lines and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> images and uses UndoManager for
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> undo/redo actions.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> He might want that it was
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> possible to work with copied
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> images. He can get lock on
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ctrl+v action, process an
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> image, prepare UndoableEdit and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> notify the UndoManager.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> He also can use lock/unlock
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> in the undo action to have a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> consistent state with the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> processed image. If someone
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> calls undo action during the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> image processing and gets a
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> deadlock does it mean that link
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> from Java Painter need to be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> added to the UndoManager?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> It looks like
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> AbstractDocument violates
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> UndoManager synchronization
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> contract when it both use
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lock to work with UndoManager
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and in the implemented undo()
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> method.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Alexandr.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the swing-dev
mailing list