On 9/11/2015 7:24 PM, Semyon Sadetsky wrote:
On 9/11/2015 6:04 PM, Alexander Scherbatiy wrote:
On 9/10/2015 7:30 PM, Semyon Sadetsky wrote:
On 9/10/2015 6:46 PM, Alexander Scherbatiy wrote:
On 9/10/2015 6:26 PM, Semyon Sadetsky wrote:
On 9/10/2015 5:13 PM, Alexander Scherbatiy wrote:
On 9/10/2015 5:07 PM, Semyon Sadetsky wrote:
On 9/10/2015 3:45 PM, Alexander Scherbatiy wrote:
On 9/10/2015 3:39 PM, Semyon Sadetsky wrote:
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?
Even though something does not work before the fix the
UndoManager has been designed to work in the multi-tread
environment and the undoOrRedo() method should work in the
predicted way.
The sample is related to singlethreaded usage of cause.
If you think that it can't be fixed with this fix you can
file a new issue that it is a problem which we aware of.
We discussed that already. The order is not deterministic
without the external fair lock. And I think it is not needed for
99.(9)% of usages. Nobody cares if an undo button pressed 1 ms
early or 1 ms later. It shouldn't be critical otherwise it is
anti-pattern. If the background editing is allowed concurrently
along the user interaction one need to use an external
synchronization or simply disable undo/redo until the editing is
done.
For me it absolutely clear that it is a degenerated scenario
when a UI document is modified concurrently from two different
threads, but we should not allow deadlocks anyway and have to
fix it.
Thanks,
Alexandr.
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.
Could you answer on the question why do you suggest so
complicated fix? Just remove the synchronization from the undo()
and redo() methods and you really fix the deadlock.
It seems that you believe that it is possible because: "If the
background editing is allowed concurrently along the user
interaction one need to use an external synchronization or simply
disable undo/redo until the editing is done."
Strange suggestion. We cannot remove synchronization it is
required to guarantee consistency.
The same is for the undoOrRedo() method. You need to provide a
synchronization for it or create an new issue on it.
undoOrRedo() just calls undo() or redo() which contain all necessary
synchronization.
UndoManager is designed as a general purpose class which can be
used from multi-thread environment even for applications which do not
use abstract documents.
Consider such application which creates its UndoableEdit events
where it does not use locks in undo() and redo() methods. This
application can use UndoManager.undo()/redo()/undoOrRedo() methods
from different threads without deadlock.
This application relies on the UndoManager.undoOrRedo()
description that the right action always will be called.
After the fix it may happen that two UndoManager.undoOrRedo()
calls can leads to two undo() calls instead of undo() and redo().
This is the regression from the previous behavior.
I showed you already that the previous behavior was a deadlock for
this scenario. No regression here.
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.
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
