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