On 9/21/2015 11:27 AM, Alexandr Scherbatiy wrote:
18.09.2015 19:22, Semyon Sadetsky пишет:
On 9/18/2015 6:51 PM, Alexander Scherbatiy wrote:
On 9/16/2015 5:14 PM, Semyon Sadetsky wrote:
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.
There are two main synchronized block in the tryUndoOrRedo()
method: one to look up an undoable edit lock and the second which
use the undoable edit lock.
In the version 02 of the fix the original code from undo() and
redo() methods were moved to these two blocks.
It is not clear why don't you want to do the same (just call
tryUndoOrRedo() with necessary argument for all
UndoManager.undo()/redo()/undoOrRedo() methods) in the latest fix?
Splitting logic like:
-------------------
420 public void undo() throws CannotUndoException {
421 if (!tryUndoOrRedo(Action.UNDO)) {
422 synchronized (this) {
423 super.undo();
424 }
425 }
426 }
-------------------
always have a question that before the fix the super.undo() was
called only for '!inProgress' condition but now the 'inProgress' can
be changed when super.undo() is called.
inProgress can be changed when super.undo() is called because it is
protected by synchronized block.
Imagine that undo() is called with inProgress = true, then it is
blocked by the concurrent document change, and after the retry it
finds inProgress= false, so it cannot undo by usual way anymore,
because the UndoManager is converted into a single edit and it should
undo using super.undo().
I am talking about slightly different thing.
This is the code for the undo() method before the fix.
-------------------
410 public synchronized void undo() throws CannotUndoException {
411 if (inProgress) {
412 UndoableEdit edit = editToBeUndone();
...
416 undoTo(edit);
417 } else {
418 super.undo();
419 }
420 }
-------------------
Checking the 'inProgress' variable and calling undoTo(edit) and
super.undo() is done under the same synchronized block.
Let's slightly modify the code:
-------------------
public void undo() throws CannotUndoException {
boolean res = true;
synchronized (this) {
if (inProgress) {
UndoableEdit edit = editToBeUndone();
...
undoTo(edit);
It is not possible to execute undoTo() here because it will cause the
deadlock we are trying to fix.
} else {
res = false;
}
}
synchronized (this) {
if (!res) {
super.undo();
}
}
}
-------------------
Now the 'inProgress' variable checking and undoTo() is done on the
first synchronized block.
The result of the inProgress is used in the second synchronized
block. But the 'inProgress' variable
can be changed between these two blocks and we can't relay on the
'res' value.
Instead of 'res' the original 'inProgress' value should be checked
in the second synchronized block and if it is true the undoTo(edit)
should be called again instead of super.undo().
Above you provided the code logic bore the fix which is reworked because
it does not work. Why do we need to discuss it?
Thanks,
Alexandr.
Thanks,
Alexandr.
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
