Bugzilla – Full Text Bug Listing
| Summary: | Add lock notifications and lock caching | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Caspar D. <caspar_d> | ||||||||||||||||||||||||||||
| Component: | cdo.core | Assignee: | Caspar D. <caspar_d> | ||||||||||||||||||||||||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | saulius.tvarijonas, smcduff, stephane.fournier, stepper | ||||||||||||||||||||||||||||
| Version: | 4.1 | Flags: | stepper:
review-
stepper: review+ stepper: review+ |
||||||||||||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||
| Whiteboard: | Power to the People | ||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||
| Bug Blocks: | 355287, 351912, 354454, 354854 | ||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Caspar D.
What I have so far, I have committed on my dev-branch: https://dev.eclipse.org/svnroot/modeling/org.eclipse.emf.cdo/branches/cdegroot-development The notifications imply that there's a client-side lockState to be updated. Currently, that lockState is not kept anywhere, unless the client app maintains its own hard references to the CDOLock instances. We'll have to add a lock cache before the notifications can be useful. Updating Bugzilla title accordingly. For apps that plan to use locks, it would be nice if the lockState(s) got loaded together with the object(s), in one signal. * Lock notifications should be optional, and disabled by default. * The protocol.lockObjects and protocol.unlockObjects should probably return the new lockStates. That would be safer and easier than changing the lockState on the client side, as that would increase the opportunity for discrepancies between the client and server side lockStates. * On the server side, locks may be held by durableViews, which have neither sessions nor viewIDs. We need to communicate something meaningful about such lock owners nevertheles... Needs thought. * Switching to a different branch should invalidate all cached lock state. (In reply to comment #7) > * Switching to a different branch should invalidate all cached lock state. And it should presumingly *release* all locks held in the old branch. Created attachment 201217 [details] Patch v1 I'm attaching a fairly complete version of this big patch, for an early review. This has only been tested against "CDO AllTests (MEM Branching)". Probably there are a few things to iron out before it'll work well with all test configs. I'll work on that. Note that this patch includes some of the work on Bug 351912. It's become hard to disentangle because I started work on that bug first, which revealed the need for notifications, caching etc... so it's all one big undertaking. Hope you don't mind. Created attachment 201219 [details]
Patch v2
Slight change so that it works with "CDO AllTests (MEM audit)"
and "CDO AllTests (MEM non-audit)" as well.
Tried "CDO AllTests (H2 branching)" too.. looks good mostly. A
few failures but I doubt they're related.
Created attachment 201222 [details]
Patch v3
I included the new classes this time... that might help ;-)
Created attachment 201253 [details]
Patch v4
The patch is quite large and I did not look at all changes for now. I reviewed the changes to cdo.common completely and focused on the cdo client then. Here are my findings so far (I've added a number of other changes in patch v4, too):
RWOLockManager
============
These should not be synchronized:
lock(LockType, CONTEXT, Collection<? extends OBJECT>, long)
lock2(LockType, CONTEXT, Collection<? extends OBJECT>, long)
lock(LockType, CONTEXT, OBJECT, long)
See the code in the method body:
// Must come before the synchronized block!
long startTime = timeout == WAIT ? 0L : currentTimeMillis();
// Do not synchronize the entire method as it would corrupt the timeout!
synchronized (this)
{
...
RWOLockManager
============
- Add IRWOLockManager interface
CDOLockInfo
========
- getOperation() should be an enum type (LOCK|UNLOCK)
- this seems to impact a bunch of other methods, too.
CDOLockState
=========
Are these supposed to be regular API:
org.eclipse.emf.cdo.common.lock.CDOLockState.setWriteLockOwner(CDOLockOwner)
org.eclipse.emf.cdo.common.lock.CDOLockState.setOptionOwner(CDOLockOwner)
CDOLockOwnerImpl
=============
We should discuss hashCode() and equals(). They look inefficient.
CDOViewImpl
=========
CDOViewImpl.OptionsImpl.setLockNotificationEnabled(boolean) does not fire an options event. Compare CDOViewImpl.OptionsImpl.setInvalidationPolicy(CDOInvalidationPolicy)
CDOLocksChangedEvent
================
Do we need CDOLocksChangedEvent.getBranch()? Can it be different from getView().getBranch()? Compare CDOViewInvalidationEvent.
(In reply to comment #13) > RWOLockManager > ============ > These should not be synchronized: > lock(LockType, CONTEXT, Collection<? extends OBJECT>, long) > lock2(LockType, CONTEXT, Collection<? extends OBJECT>, long) > lock(LockType, CONTEXT, OBJECT, long) Fixed accordingly. > RWOLockManager > ============ > - Add IRWOLockManager interface Done. > CDOLockInfo > ======== > - getOperation() should be an enum type (LOCK|UNLOCK) > - this seems to impact a bunch of other methods, too. Done. > CDOLockState > ========= > Are these supposed to be regular API: > org.eclipse.emf.cdo.common.lock.CDOLockState.setWriteLockOwner(CDOLockOwner) > org.eclipse.emf.cdo.common.lock.CDOLockState.setOptionOwner(CDOLockOwner) No, you're right, they shouldn't be. I've created InternalCDOLockState and moved them there. But that still left the readLockOwners exposed through the regular API, an ugly inconsistency that I addressed by adding add/removeReadLockOwner to the SPI interface, and returning an unmodifiable set from the getter in the regular interface. > CDOLockOwnerImpl > ============= > We should discuss hashCode() and equals(). They look inefficient. Re hashCode: replaced it with something simple but effective. Re equals: How is a comparison of 2 integers "inefficient"? Or do you mean the method calls to fetch those integers? Eliminating the latter is only possible by requiring the "other" object to be of the same implementation class -- and I actually prefer such strictness but I thought it would run contrary to your penchant for interfaces... > CDOViewImpl > ========= > CDOViewImpl.OptionsImpl.setLockNotificationEnabled(boolean) does not fire an > options event. Compare > CDOViewImpl.OptionsImpl.setInvalidationPolicy(CDOInvalidationPolicy) Added the notification. Enhanced test to verify the event. > CDOLocksChangedEvent > ================ > Do we need CDOLocksChangedEvent.getBranch()? Can it be different from > getView().getBranch()? Compare CDOViewInvalidationEvent. No, it can't be different. I've removed it. Attaching new patch in a minute. Created attachment 201289 [details]
Patch v5
TODO: Add tests for NEW and TRANSIENT objects. Created attachment 201291 [details]
Patch v6
Legacy tests now passing.
AllTests now passing. (There's one failure but it's not caused by this patch.) Thanks for addressing my comments so quickly. I'm rejecting the review now just so that you're able to request it again later.
Regarding the efficiency of hashCode() and equals() I was mostly referring to the hashCode() method. But I noticed that your equals method differs slightly from our "standard" idiom, e.g.,
@Override
public boolean equals(Object obj)
{
if (obj == this)
{
return true;
}
if (obj instanceof CDOBranch)
{
CDOBranch that = (CDOBranch)obj;
return id == that.getID();
}
return false;
}
Created attachment 201293 [details]
Patch v7
Patches v5 and v6 didn't include the new files... again.
(In reply to comment #19) > Regarding the efficiency of hashCode() and equals() I was mostly referring to > the hashCode() method. But I noticed that your equals method differs slightly > from our "standard" idiom, e.g., OK, rewritten to follow the idiom exemplified by CDOBranchImpl. Created attachment 201294 [details]
Patch v8
Created attachment 201307 [details]
Patch v9
Addressed my own TODOs. I think it's ready now, flagging
for review again.
Created attachment 201560 [details]
Patch v10
The patch looks very good and all tests are passing. I approve this patch and take your word that you add JavaDoc to these items before you commit it:
CDOLockOwner
CDOLockUtil
CDOObject.cdoLockState()
CDOLocksChangedEvent
CDOView.Options.isLockNotificationEnabled()
CDOView.Options.setLockNotificationEnabled(boolean)
Committed revision 8908. Created attachment 201686 [details]
Patch v11 (incremental)
Discovered 2 gross errors:
* While the notifications work, the new lockStates are not actually
processed locally. The fix for this is a trivial one-liner.
* LockManager.unlock2 updates the persisted lockState before calling
super.unlock2. But super.unlock2 might fail. The order should be
the other way around. Here too, the fix is trivial.
I enhanced LockingNotificationsTest and moved the test listener that
was contained there, to o.e.e.cdo.tests.util
Reopening for a review of the additional patch. Created attachment 201687 [details]
Patch v12 (incremental)
Patched the patch.
By accident I just found out that there are several public overrides of synchronized super methods that are themselves not synchronized. I mean in org.eclipse.emf.cdo.internal.server.LockManager. Can you please check that? Committed revision 8912: - trunk/plugins/org.eclipse.emf.cdo.tests Can you remember why org.eclipse.emf.cdo.tests.offline.OfflineLockingTest.testMasterLocks_ArrivalInClone() is empty? Created attachment 201690 [details]
Patch v13 (incremental)
I've synchronized these methods:
LockManager.unlock(boolean, LockType, IView, Collection<? extends Object>)
LockManager.unlock2(boolean, LockType, IView, Collection<? extends Object>)
LockManager.unlock(boolean, IView)
LockManager.unlock2(boolean, IView)
(In reply to comment #31) > Can you remember why > org.eclipse.emf.cdo.tests.offline.OfflineLockingTest. > testMasterLocks_ArrivalInClone() > is empty? I'll implement it as part of Bug 351912. Committed revision 8915. (In reply to comment #29) > By accident I just found out that there are several public overrides of > synchronized super methods that are themselves not synchronized. I mean in > org.eclipse.emf.cdo.internal.server.LockManager. Can you please check that? Which methods? I didn't find any that are unsynced while overriding a synced method. (In reply to comment #35) > (In reply to comment #29) > > By accident I just found out that there are several public overrides of > > synchronized super methods that are themselves not synchronized. I mean in > > org.eclipse.emf.cdo.internal.server.LockManager. Can you please check that? > > Which methods? I didn't find any that are unsynced while overriding a > synced method. The ones from comment 32. I fixed that already in the latest patch! Closing. *** Bug 283274 has been marked as a duplicate of this bug. *** *** Bug 254696 has been marked as a duplicate of this bug. *** *** Bug 273411 has been marked as a duplicate of this bug. *** |