Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1106464 - if getReturnValue().setSucceeded(true); is called before exception is thrown command is presented as successful.
Summary: if getReturnValue().setSucceeded(true); is called before exception is thrown ...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: ovirt-engine
Version: 3.5.0
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: ---
: 3.5.0
Assignee: Ravi Nori
QA Contact: Martin Mucha
URL:
Whiteboard: infra
Depends On:
Blocks: rhev3.5beta 1156165
TreeView+ depends on / blocked
 
Reported: 2014-06-09 12:15 UTC by Martin Mucha
Modified: 2016-02-10 19:38 UTC (History)
12 users (show)

Fixed In Version: ovirt-engine-3.5.0_beta
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-02-17 17:07:32 UTC
oVirt Team: Infra
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
oVirt gerrit 28916 master MERGED engine : if setSucceeded(true) is called before exception command is presented as succeeded Never

Description Martin Mucha 2014-06-09 12:15:16 UTC
Description of problem:

if any Command calls:
getReturnValue().setSucceeded(true);
and after that follows some logic, then if this logic fails, outcome of this command will be presented as successful. Compensation/rollbacks etc. are called, but data sent to user are wrong.

example: 
if commands ends with:

if (1 == 1) throw new RuntimeException();
getReturnValue().setSucceeded(true);

rest will return:

<fault>
    <reason>Operation Failed</reason>
    <detail>[Internal Engine Error]</detail>
</fault>

but if command would be like this:

getReturnValue().setSucceeded(true);
if (1 == 1) throw new RuntimeException();


result will be:

<action>
    <status>
        <state>complete</state>
    </status>
</action>


How reproducible:
100%


Actual results:
see above.

Expected results:
command failed in both scenarios and returned data should reflect that.

Comment 1 Oved Ourfali 2014-06-12 06:27:30 UTC
I think that it should be the responsibility of the command to call setSucceeded(true) only once nothing is left to do... in my opinion, commands that do further logic after that assumes that this logic is insignificant for the successful completion of the command.

Yair/Ravi - thoughts?

Comment 2 Martin Mucha 2014-06-12 07:35:45 UTC
I think, on contrary, that if somewhere there need not to be some convention(setSucceed must be last call of command) or faith in assumptions("insignificant" code will not fail), there should not be such things.

Exception can occur unexpectedly, and so does db rollback. Flagging each command from which exception 'leaks' as unsuccessful will spell more robust architecture — user should not get info "OK" if it's not OK, and we should not rely on each command to behave as expected to provide user right response. It's command 'invoker' responsibility.

Comment 3 Yair Zaslavsky 2014-06-12 08:01:36 UTC
I agree with Martin.
Bare in mind that execute is run from within CommandBase, so I think we should handle it in CommandBase.

Comment 4 Petr Beňas 2014-10-21 15:37:35 UTC
Code change...

Comment 5 Eyal Edri 2015-02-17 17:07:32 UTC
rhev 3.5.0 was released. closing.


Note You need to log in before you can comment on or make changes to this bug.