Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 352977

Summary: Dirty Objects of CDOTransaction with CDOSavepoint
Product: [Modeling] EMF Reporter: Steve Monnier <steve.monnier>
Component: cdo.coreAssignee: Eike Stepper <stepper>
Status: CLOSED WORKSFORME QA Contact: Eike Stepper <stepper>
Severity: normal    
Priority: P3 CC: alex.lagarde, esteban.dugueperoux, stephane.fournier
Version: 4.2   
Target Milestone: ---   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Bug Depends on: 312534    
Bug Blocks:    
Attachments:
Description Flags
Fix v1 (incomplete)
none
Patch v2 (still wrong)
none
Workaround that needs method visibility change none

Description Steve Monnier CLA 2011-07-25 04:22:38 EDT 
Build Identifier: 

Hello,

I have an issue with dirty objects of a CDOTransaction using CDOSavepoint. It seems to be caused by CDOTransactionImpl.getDirtyObjects() that calls lastSavepoint.getAllDirtyObjects();

Here is my situation :
I have a TransactionalEditingDomainListener that creates a CDOSavepoint on transactionClosed for undo management.

I have an issue with the following scenario :
- I edit a CDOObject but don't commit -> A CDOSavepoint is created with this CDOObject dirty
- I delete this same CDOObject -> A CDOSavepoint is created with this CDOObject detached
- I Commit
-> because CDOTransactionImpl.getDirtyObjects() calls lastSavepoint.getAllDirtyObjects(); that returns dirty objects of all CDOSavepoint (If I'm right) instead of getDirtyObjects, my object is both detached and dirty on commit, causing NPE later in CDOCommitContextImpl.collectLobs(CDOTransactionImpl.java:2578)


I have two stack traces for this issue:
 - The first one has message : Impossible to commit changes made on resource '*my_resource*'on the repository
org.eclipse.emf.cdo.util.CommitException: java.lang.NullPointerException
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.commit(CDOTransactionImpl.java:1072)
at org.eclipse.emf.cdo.transaction.CDOPushTransaction.push(CDOPushTransaction.java:245)
at org.eclipse.emf.cdo.transaction.CDOPushTransaction.push(CDOPushTransaction.java:240)
...
my SavingPolicy
...
Caused by: java.lang.NullPointerException
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl$CDOCommitContextImpl.collectLobs(CDOTransactionImpl.java:2578)
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl$CDOCommitContextImpl.preCommit(CDOTransactionImpl.java:2570)
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl$CDOCommitContextImpl.preCommit(CDOTransactionImpl.java:2415)
at org.eclipse.emf.internal.cdo.transaction.CDOSingleTransactionStrategyImpl.commit(CDOSingleTransactionStrategyImpl.java:66)
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.commit(CDOTransactionImpl.java:1058)

- The second stack trace is a warning with message : "Save Failed"
java.lang.IllegalStateException: Failing event ROLLBACK in state CLEAN for Car@OID9 (data=null)
at org.eclipse.net4j.util.fsm.FiniteStateMachine.process(FiniteStateMachine.java:153)
at org.eclipse.emf.internal.cdo.view.CDOStateMachine.rollback(CDOStateMachine.java:448)
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.rollbackCompletely(CDOTransactionImpl.java:1174)
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.handleRollback(CDOTransactionImpl.java:1369)
at org.eclipse.emf.internal.cdo.transaction.CDOSingleTransactionStrategyImpl.rollback(CDOSingleTransactionStrategyImpl.java:118)
at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.rollback(CDOTransactionImpl.java:1087)
...
My SavingPolicy

Thanks,
Steve

Reproducible: Always

Steps to Reproduce:
1. edit a CDOObject but don't commit -> A CDOSavepoint is created with this CDOObject dirty
2. delete this same CDOObject -> A CDOSavepoint is created with this CDOObject detached
3. Commit
Comment 1 Steve Monnier CLA 2011-07-27 04:52:32 EDT 
When you will have a fix, I will be interested to have it on the 4.0.X maintenance version, if possible.

Thank you,
Steve
Comment 2 Eike Stepper CLA 2011-07-27 04:55:11 EDT 
One step after the other ;-)
Comment 3 Steve Monnier CLA 2011-07-27 05:02:56 EDT 
Of course :)
Comment 4 Eike Stepper CLA 2011-07-27 09:52:26 EDT 
Created attachment 200450 [details]
Fix v1 (incomplete)

Seems to fix the problem but breaks rollback()
Comment 5 Eike Stepper CLA 2011-07-28 03:13:42 EDT 
Committed revision 8836:
- trunk/plugins/org.eclipse.emf.cdo
Comment 6 Eike Stepper CLA 2011-07-28 03:14:41 EDT 
Committed revision 8838:
- trunk/plugins/org.eclipse.emf.cdo
Comment 7 Eike Stepper CLA 2011-07-28 03:19:14 EDT 
Created attachment 200494 [details]
Patch v2 (still wrong)

As also explained on bug 353167 savepoints are not in a very good shape with respect to detachment or reattachment and it's very hard to change. It seems that a major rework is pending and I'm a little reluctant to spend much effort on this old design. I'll come back to this bugzilla when the big redesign takes place. Sorry for the inconvenience!
Comment 8 Steve Monnier CLA 2011-10-25 06:05:28 EDT 
Created attachment 205900 [details]
Workaround that needs method visibility change

I found a workaround by calling the collapseSavepoints method of the CDOTransactionImpl before calling SetSavepoint. However, collapseSavepoints method is private. Is it possible to have this method as API like in this patch. Is this workaround acceptable for 4.0_Maintenance branch?

Thanks
Comment 9 Eike Stepper CLA 2011-11-12 01:46:23 EST 
Steve, your patch is against 4.0 while this bug is against 4.1. Generally we're trying to back-port bug fixes but changes to public interfaces are generally not allowed in maintenance streams. We could back-port the change to CDOTransactionImpl, though, and you'd have to down-cast your tx handle.

Out of curiosity, How do you mimic the CDOCommitContext needed when calling collapseSavepoints()?
Comment 10 Steve Monnier CLA 2011-11-14 05:34:23 EST 
(In reply to comment #9)
> Steve, your patch is against 4.0 while this bug is against 4.1. 
Sorry my bad, I must have missed it during the creation of this issue.

>Generally we're
> trying to back-port bug fixes but changes to public interfaces are generally
> not allowed in maintenance streams. We could back-port the change to
> CDOTransactionImpl, though, and you'd have to down-cast your tx handle.
Ok, it's good for me.

> Out of curiosity, How do you mimic the CDOCommitContext needed when calling
> collapseSavepoints()?
Well, we are in a TransactionalEditingDomainListener that creates a
CDOSavepoint on transactionClosed for undo management. 
After initialization of this TransactionalEditingDomainListener, we create and
keep an InternalCDOCommitContext by calling
InternalCDOTransaction.createCommitContext();
This way, in transactionClosed(), if we are not dealing with a rollback, we can
collapseSavepoints before calling setSavepoint() on the CDOTransaction.
I hope it is not an heresy, but this workaround seems to work fine, in our case at least.
Comment 11 Eike Stepper CLA 2011-11-15 01:42:46 EST 
Steve, this is at least dangerous because the constructor of CDOCommitContextImpl calls calculateCommitData() only once. The transaction is not supposed to be used between the commit context creation and the end of the commit operation.

Please note that Thales is currently considering to provide some budget for the general savepoint redesign I've mentioned in comment #7.
Comment 12 Eike Stepper CLA 2012-08-14 22:57:14 EDT 
Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master.
Comment 13 Steve Monnier CLA 2013-02-19 09:22:40 EST 
I can't reproduce the issue of the initial scenario on CDO 4.2. I guess you can close this one.
Comment 14 Eike Stepper CLA 2013-03-19 04:51:25 EDT 
Ok. Thanks for letting me know ;-)