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.
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
