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