Modify

Opened 5 months ago

Last modified 3 weeks ago

#14526 new defect

SequenceCommand doesn't rollback properly in case of subcommand error.

Reported by: Tyndare Owned by: team
Priority: normal Milestone: 17.08
Component: Core Version:
Keywords: SequenceCommand undo Cc: Don-vip, bastiK, michael2402

Description

Hi,

I think found an issue in the SequenceCommand class: if one of the sub-commands fails, the preceding sub-commands that were already executed before are not undone.
This seems to contradict the code intention.

I also have problems with the continueOnError variable of this class. I tried to make a test (see the attached file) but I was not able to make it work.
Actually I'm not sure I fully understand what is the contract between executeCommand() and undo().
Is it ok/necessary/forbidden to call undo() if execucedCommand() returned false ?

Thanks.

Attachments (2)

SequenceCommandTest.java.patch (2.5 KB) - added by Tyndare 5 months ago.
Updated version of the [PATCH] for SequenceCommandTest.java
Command-execution-status.diff (4.7 KB) - added by Tyndare 5 months ago.
Documentation and UndoRedoHandler modif for Command.executeCommand() return status.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 5 months ago by stoecker

Cc: Don-vip bastiK added
Milestone: 17.03

Hmm.

        for (int i = 0; i < sequence.length; i++) {
            boolean result = sequence[i].executeCommand();
            if (!result && !continueOnError) {
                undoCommands(i-1);
                return false;
            }
        }

Code looks ok to me - why do you think they aren't undone?

You have to check UndoRedoHandler.java to understand the Command logic, as every command should pass this class.

But when I look at it, then the result of c.executeCommand() is not checked in addNoRedraw(). That's strange.

I'd expect the failed commands aren't added to the undoRedo list and thus also never can be undone.

Hmm.

comment:2 Changed 5 months ago by stoecker

P.S. "success" has two "c" :-)

comment:3 Changed 5 months ago by Tyndare

I know the code looks ok, that's why I provided a test that show it doesn't do what it tells it is doing....

The call to

undoCommands(i-1);

will do nothing because this function has a test

if (!sequenceComplete)
    return;

comment:4 Changed 5 months ago by Tyndare

I looked at the class UndoRedoHandler.java and at the javadoc of the methods executeCommand() and undo() in the class Command.java

They are coherent:

  • undo(): It can be assumed that executeCommand was called exactly once before.
  • executeCommand(): return true

I understand it better now: things are never supposed to fail ;-)
Unfortunately it is difficult to ensure this when composing big sequence of commands that could impact one another.

So if I understand:

  • if executeCommand() returned true, it is mandatory to call undo() to consider the command undone.
  • if executeCommand() returned false, it should be safe but not mandatory to call undo(), the command should have already undone itself() and let an unmodified state.

Am I correct ?

comment:5 Changed 5 months ago by stoecker

Well. That code was probably not touched for a LOOOOOONG time.

  • I think the executeCommand() call in undoHandler should check the return value and at least return it to the caller.
  • The executeCommand() should leave a state that is either true and changed or false and unchanged.
  • Undo() should undo a successful executeCommand() or nothing for an unsuccessful call (assuming that an unsuccessful executeCommand() already did cleanup).
  • The documentation should be explicit about this.
  • The undoCommands() in SequenceCommand.java is broken for sure in case of a subcommand failure. The check should go to the public call instead.

We have sometimes issues which could be caused by misbehaviour in the undo/redo handling.

comment:6 Changed 5 months ago by stoecker

In 11733/josm:

see #14526 - fix error handling in SequenceCommand

Changed 5 months ago by Tyndare

Updated version of the [PATCH] for SequenceCommandTest.java

comment:7 Changed 5 months ago by Tyndare

Thanks for the explanations and the fix. I've updated the patch to test continueOnError accordingly.

comment:8 Changed 5 months ago by stoecker

In 11734/josm:

improve SequenceCommandTest, see #14526 - patch by Tyndare

comment:9 Changed 5 months ago by Don-vip

why is the ticket not closed?

comment:10 Changed 5 months ago by stoecker

Because this things of comment 5 are still unsolved.

comment:11 Changed 5 months ago by Tyndare

Hi,

I looked at the Command code a little more.

I have the impression that the Command class is implementing a little more than what is expected from the UndoRedoHandler.

In particular, the Command class is forcing subclasses to implement fillModifiedData(), but this method is used absolutely nowhere in the code except from the default implementation of executeCommand(), and half of Command subclasses override executeCommand(), so in practice their implementation of fillModifiedData() is never called.

Most Commands seems to have implemented fillModifiedData() correctly though, but there is one exception, the class: TransformNodesCommand
In the test it is said that the result is "intentionally empty", but why is it so important that a method which is never called return an empty result, in contradiction to the function specification ?

On the contrary, the method getParticipatingPrimitives() seems to be really used. But the default implementation provided by the Command class is only valid if the default implementation of executeCommand() has been called, in some cases executeCommand() has been overridden but not getParticipatingPrimitives(), this is the case for the classes :

Though the test SelectCommandTest explicitly test that no primitive are participating to the command, I don't understand why the selected elements should not be considered participating in the SelectCommand.

If I would be radical, I would remove the default implementation of executeCommand() and undoCommand() and getParticipatingPrimitives(), makes them abstract, and create a new subclass called StateCommand with these implementations for the classes that really use them, I would also move the field cloneMap and the inner class CloneVisitors, and the methods fillModifiedData() and getOrig() inside this new SateCommand class.
The problem is that it would break plugins.

Anyway please find attached a small patch following I hope what was said by stoecker in comment #5.

Maybe a failing command should not be added to the command queue, but we must be sure that the user has been notified of the failure if we do so, it’s not clear for me who should do that, the command itself, the UndoRedoHandler, or the code that added the command to the UndoRedoHandler.

Changed 5 months ago by Tyndare

Documentation and UndoRedoHandler modif for Command.executeCommand() return status.

comment:12 Changed 5 months ago by stoecker

While breaking plugins is not really nice when it helps us to get rid of these "sometimes" errors by a cleaner design I'd do it. Thought I prefer a way which allows a deprecation instead of a hard break (i.e. a cleaner new interface and a deprecation of the old one and step by step plugin fixing).

Failures in the command queue should always be handled by the calling code. Only there is clear if the user must be informed or if so other action can be taken. As you already observed it is not really meant to fail.

Vincent, your opinion?

comment:13 Changed 5 months ago by Don-vip

I agree to cleanup/improve the Commands classes :)
But I'd like to finish remaining stuff of gsoc-core first before starting a new cycle of updating/fixing plugins, i.e retrieving a stable Jenkins build, 0 Java warnings and wait for correction of #14528, #14495, #14120, #13999, #13665, #13604, #13309.

We still get a lot of bug reports about plugins not handling correctly all the checks implemented last summer and it takes a lot of time to review/triage those tickets. I don't want to suddenly increase the number of bug reports before we fix this.

Last edited 4 months ago by Don-vip (previous) (diff)

comment:15 Changed 5 months ago by stoecker

Milestone: 17.0317.04

comment:16 Changed 4 months ago by Don-vip

Milestone: 17.0417.05

There is still some work to do to fix last gsoc-core open points (mainly, fixing deprecation warnings)

comment:17 Changed 4 months ago by bastiK

The remaining deprecation warnings (#14120) are very minor. While it may be preferable to get rid of all deprecations, those are not actually a problem or reason for concern. Certainly it shouldn't stop progress in other tickets, like this one.

What Tyndare suggests in comment 11 makes a lot of sense to me and I would be interested to see a patch in that direction. It should be possible to handle errors in commands and SequenceCommand in a very controlled manner and not leave an undo-able mess behind.

comment:18 Changed 3 months ago by Don-vip

Milestone: 17.0517.06

comment:19 Changed 8 weeks ago by Don-vip

Milestone: 17.0617.07

comment:20 Changed 3 weeks ago by Don-vip

The situation is a lot more stable now, I see no more problem to improve the JOSM commands :)

Michael: is #13036 similar?

comment:21 Changed 3 weeks ago by Don-vip

Cc: michael2402 added
Milestone: 17.0717.08

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The owner will remain team.
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from team to the specified user. Next status will be 'new'.
Next status will be 'needinfo'.The owner will change to Tyndare
as duplicate The resolution will be set to duplicate. Next status will be 'closed'.The specified ticket will be cross-referenced with this ticket
The owner will be changed from team to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.