Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 355045 - Allow NEW objects to be locked on commit
Summary: Allow NEW objects to be locked on commit
Status: CLOSED DUPLICATE of bug 387563
Alias: None
Product: EMF
Classification: Modeling
Component: cdo.core (show other bugs)
Version: 4.6   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Caspar D. CLA
QA Contact: Eike Stepper CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-18 05:27 EDT by Caspar D. CLA
Modified: 2020-12-11 10:33 EST (History)
3 users (show)

See Also:
stepper: review+


Attachments
Patch v1 (11.16 KB, patch)
2011-09-22 07:00 EDT, Caspar D. CLA
no flags Details | Diff
Patch v2 (29.91 KB, patch)
2011-09-26 23:49 EDT, Caspar D. CLA
no flags Details | Diff
Patch v3 (36.50 KB, patch)
2011-09-27 03:57 EDT, Caspar D. CLA
no flags Details | Diff
Patch v4 (38.56 KB, patch)
2011-09-27 06:58 EDT, Caspar D. CLA
no flags Details | Diff
Patch v5 (43.45 KB, patch)
2011-09-28 07:52 EDT, Caspar D. CLA
no flags Details | Diff
Patch v6 (43.35 KB, patch)
2011-09-28 07:57 EDT, Caspar D. CLA
no flags Details | Diff
Patch v7 (46.93 KB, patch)
2011-09-29 02:02 EDT, Eike Stepper CLA
no flags Details | Diff
Patch v8 (46.38 KB, patch)
2011-09-29 05:38 EDT, Caspar D. CLA
no flags Details | Diff
Patch v9 (53.21 KB, patch)
2011-10-06 04:56 EDT, Caspar D. CLA
no flags Details | Diff
Patch v10 (60.62 KB, patch)
2011-10-06 06:52 EDT, Caspar D. CLA
no flags Details | Diff
Patch v11 (62.03 KB, patch)
2011-10-06 07:22 EDT, Caspar D. CLA
no flags Details | Diff
Patch v12 (89.47 KB, patch)
2011-10-07 04:13 EDT, Eike Stepper CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caspar D. CLA 2011-08-18 05:27:25 EDT
Currently a NEW object can only be locked by committing it
first, thus transitioning it to CLEAN state, and then locking
it.

It would be nice if NEW objects could be locked right away.

Implementation seems fairly straightforward: NEW objects should
have a real lockState object instead of the NOOPLockState than they 
have now. If any locks are configured, they need to be transmitted
during commit and entered into the server-side lockManager.
Comment 1 Caspar D. CLA 2011-09-22 07:00:55 EDT
Created attachment 203833 [details]
Patch v1
Comment 2 Caspar D. CLA 2011-09-26 06:56:21 EDT
A few subtleties to consider:

- When notifying other sessions, the commitNotification should
  be sent before the lockNotification, otherwise the lockNotification
  will reference new objects that those other sessions haven't yet
  been informed about. (Ideally the data and lock notifications
  should be combined into one signal in such situations, but perhaps
  that's best undertaken as a separate effort.)

- When autoReleaseLocks is enabled, what should happen when a session
  commits a new object with a lock? Nothing, I suppose: he locks and
  unlocks the object with the same operation, the net effect being
  no change on the server. On the client however, the lock still needs
  to be removed from the new object's lockState.
Comment 3 Caspar D. CLA 2011-09-26 23:49:34 EDT
Created attachment 204046 [details]
Patch v2
Comment 4 Caspar D. CLA 2011-09-27 03:57:08 EDT
Created attachment 204054 [details]
Patch v3
Comment 5 Caspar D. CLA 2011-09-27 06:58:28 EDT
Created attachment 204065 [details]
Patch v4
Comment 6 Caspar D. CLA 2011-09-28 07:52:35 EDT
Created attachment 204169 [details]
Patch v5
Comment 7 Caspar D. CLA 2011-09-28 07:53:12 EDT
Turned out more complicated than I expected. Still, seems to work.
Comment 8 Caspar D. CLA 2011-09-28 07:57:05 EDT
Created attachment 204170 [details]
Patch v6
Comment 9 Eike Stepper CLA 2011-09-29 00:57:25 EDT
(In reply to comment #2)
> - When notifying other sessions, the commitNotification should
> be sent before the lockNotification, otherwise the lockNotification
> will reference new objects that those other sessions haven't yet
> been informed about. (Ideally the data and lock notifications
> should be combined into one signal in such situations, but perhaps
> that's best undertaken as a separate effort.)

It should be a separate effort but we shouldn't delay it unnecessarily.
I think even if the lock notification is sent after the commit notification 
it could easily overtake the latter on the wire. Or do the lock notifications 
participate in the outOfSequence ordering of session invalidations?

> - When autoReleaseLocks is enabled, what should happen when a session
> commits a new object with a lock? Nothing, I suppose: he locks and
> unlocks the object with the same operation, the net effect being
> no change on the server. On the client however, the lock still needs
> to be removed from the new object's lockState.

Doesn't that apply to all lock states after commit with autoReleaseLocks?
Comment 10 Eike Stepper CLA 2011-09-29 02:02:15 EDT
Created attachment 204243 [details]
Patch v7

I've refactored (removed) CDOObjectWrapper.createCDOLock(). Same for CDOObjectWrapper.cdoLockState().

CDODataOutputImpl.writeCDOCommitData() and CDODataInputImpl.readCDOCommitData() do not consider locksOnNewObjects. That needs clarification before I can approve the review.
Comment 11 Caspar D. CLA 2011-09-29 05:38:56 EDT
Created attachment 204276 [details]
Patch v8
Comment 12 Caspar D. CLA 2011-10-06 04:56:35 EDT
Created attachment 204656 [details]
Patch v9

Reworked as per your proposal: the locksOnNewObjects collection is
now gettable from the CDOCommitContext iface, instead of the
CDOCommitData iface. Methods on CDOSessionProtocol adjusted to 
receive this collection as an argument.
Comment 13 Caspar D. CLA 2011-10-06 06:52:43 EDT
Created attachment 204658 [details]
Patch v10

Offline broken.
Comment 14 Caspar D. CLA 2011-10-06 07:22:13 EDT
Created attachment 204659 [details]
Patch v11

Offline fixed.
Comment 15 Eike Stepper CLA 2011-10-07 04:13:44 EDT
Created attachment 204729 [details]
Patch v12

- I've removed the getLocksOnNewObjects() method from all (2) CDOCommitData implementations
- I've replaced the implementation of the deprecated methods in CDOClientProtocol by UnsupportedOperationException
- I've replaced the UnsupportedOperationException in CDOCommitContextImpl.getUserID() with return transaction.getSession().getUserID()
- I've moved the new methods from InternalCDOCommitContext to CDOCommitContext
- Throwing UnsupportedOperationException from CommitDelegationRequest.getObjectType(CDOID)

One failure in suite "H2 All" (all scenarios):

junit.framework.AssertionFailedError: expected:<true> but was:<false>
	at org.eclipse.emf.cdo.tests.LockingManagerRestartTransactionTest.testDurableViewHandler(LockingManagerRestartTransactionTest.java:391)
Comment 16 Eike Stepper CLA 2011-10-07 04:19:11 EDT
I'm approving the patch but please look at (fix) the failing test case ;-)
Comment 17 Caspar D. CLA 2011-10-10 01:06:11 EDT
Committed revision 9469.

Concerning the test failure: the problem is that ConfigTest.restartRepository
doesn't really restart the repository, but replaces it. The RepoConfig puts
a listener on every repo that gets registered (see RepositoryConfig.
registerRepository), and this listener removes the repo from the config's
map of repositories. The subsequent getRepo(String) name call, triggers
instantation of a new repository, but of course the listener that was added
by the TEST logic (not to be confused with the listener I just mentioned),
is still attached to (only) the original repo, which was deactivated and
discared.

Possible solutions:

1. Disable this test for repository restarts

2. Fix the test logic so that restartRepo *restarts* a repo. This might
   be difficult. Of course the logic in the handler can be made conditional
   on isRestarting() -- that's trivial. But I don't think repos and all
   their constituent parts are prepared for being restarted. The assumption
   seems to have been that after a #deactivate call, the objects will not be
   used again. I'm not sure how much work it will be to address this.
Comment 18 Esteban DUGUEPEROUX CLA 2012-08-13 03:32:25 EDT
Hi Caspar,

Could I have your advice about http://www.eclipse.org/forums/index.php/t/368113/ ? We have a similar need to this one where we have issue on other users lock/commit notifications reception ordering.
Comment 19 Eike Stepper CLA 2012-08-14 22:52:04 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 20 Esteban DUGUEPEROUX CLA 2012-08-20 03:57:59 EDT
Working on https://bugs.eclipse.org/bugs/show_bug.cgi?id=387563 to generalize this feature, I see that the recursive parameter is not taken into account, because If I attach a new object containment tree and I choose to lock this object containment tree with recursive parameter at true, after commit only the root of this object containment tree will be locked.

I will fix that iterating with eAllContents() to register all objects to lock/unlock on commit.
Comment 21 Esteban DUGUEPEROUX CLA 2012-08-21 08:11:57 EDT
There is also the case where we lock recursively a object tree having objects with clean state except for a subtree for which objects are in NEW state. In this case only the objects in CLEAN state are locked and not the objects in NEW state, event after a commit. In conclusion the feature of recursive locking (CDOSessionProtocol.lockObjects2() https://bugs.eclipse.org/bugs/show_bug.cgi?id=354454 ) is not compatible with this feature of lock new objects on commit. To fix that we must iterate on the tree to see if there is object in NEW state. Then the CDOSessionProtocol.lockObjects2() operation become useless. I will manage this use case in https://bugs.eclipse.org/bugs/show_bug.cgi?id=387563 .
Comment 22 Eike Stepper CLA 2013-06-27 04:07:06 EDT
Moving all outstanding enhancements to 4.3
Comment 23 Eike Stepper CLA 2014-08-19 09:25:15 EDT
Moving all open enhancement requests to 4.4
Comment 24 Eike Stepper CLA 2014-08-19 09:36:06 EDT
Moving all open enhancement requests to 4.4
Comment 25 Eike Stepper CLA 2015-07-14 02:11:46 EDT
Moving all open bugzillas to 4.5.
Comment 26 Eike Stepper CLA 2016-07-31 00:54:18 EDT
Moving all unaddressed bugzillas to 4.6.
Comment 27 Eike Stepper CLA 2016-09-24 00:57:44 EDT

*** This bug has been marked as a duplicate of bug 387563 ***
Comment 28 Eike Stepper CLA 2020-12-11 10:25:22 EST
Closing.
Comment 29 Eike Stepper CLA 2020-12-11 10:33:26 EST
Closing.