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