Comment 1 Caspar D. 2010-07-20 06:49:36 EDT

Created attachment 174723 [details]
Patch

*** Patch adopted from bug 319836 ***
(Modified to avoid API breakage.)

Comment 2 Eike Stepper 2010-07-20 07:28:12 EDT

Created attachment 174727 [details]
Patch v2 - ready to be committed

I mostly added some instanceof CDOTransactionImpl checks.

Comment 3 Caspar D. 2010-07-20 22:35:31 EDT

Yes, I see. But I have my doubts about those.

Consider this for example, from CDOStateMachine.java, lines
207-215:

if (transaction instanceof CDOTransactionImpl && 
   ((CDOTransactionImpl)transaction).getFormerRevisionKeys().containsKey(object))
{
  reattachObject(object, transaction);
}
else
{
  attachObject(object);
}

In your version, the condition that the transaction be an
instance of CDOTransactionImpl, affects the decision whether
to reattach or attach. This changes the meaning of the
'else' branch. That is, the else branch gets executed not
only for the case that the revisionKey is not found, but
also for the case the tx isn't a CDOTransactionImpl. In the
latter case, we are inadvertently covering up an error
state.

I prefer my original code, which will cause an exception...

Comment 4 Eike Stepper 2010-07-21 01:51:13 EDT

My idea was that reattach can not be supported at all if the tx is not a CDOTransactionImpl. But it's well possible that other places must be touched for this, too.

An exception is also okay for me. Do you want to change it? No review needed...

Comment 5 Caspar D. 2010-07-28 02:41:50 EDT

Committed to R3_0_maintenance

Comment 6 Eike Stepper 2011-06-23 04:27:24 EDT

Moving all open problem reports to 4.0

Comment 7 Eike Stepper 2012-09-21 06:36:59 EDT

Undoing accidental version change.

Comment 8 Eike Stepper 2012-09-21 06:52:00 EDT

Closing.