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