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