Community
Participate
Working Groups
When a revision is first made dirty in a TX, we copy the revision and have the new copy carry the new state. The revision that was originally loaded, isn't altered -- nor should it be, IMO. But we have no mechanism in place that actually ensures that revisions are used in this way. I propose that we add a method #setImmutable() that we call on revisions right after they are loaded, and add some checks to methods that alter the state of the revision, to throw an exception if any of those methods are called on a revision object that's been toggled to be immutable.
+1
Created attachment 194919 [details] Patch v1 Simple implementation of the freezing functionality. Thoughts: - This probably doesn't protect list features from being changed, because those can be retrieved from the revision and then modified without the rev object knowing about it. Probably the lists will have to be frozen as well. - The freeze method will have to be called at various places: right after loading on the client side; ditto on the server side; after commit on the client side (i.e. dirty->clean transition). Perhaps more? - Revising a revision is not a violation of its frozen state. The frozen- ness pertains only to the feature values.
Created attachment 194920 [details] Patch v2 Added logic to allow some exceptional cases to pass. Added invocation of #freeze after loading a rev on the client side.
So far this frozen revisions have revealed one bug (I think): OCLQueryTest.testDirtyObject fails. The CDOChangeSetDataRevisionProvider used there makes changes to revisions that it obtains from a delegate (another CDORevisionProvider) without making a copy of the revision first. This doesn't seem right to me. In the case of OCLQueryTest.testDirtyObject the revision provider is a transaction, but logic outside the tx shouldn't assume that revisions can just be modified without involving all the TX and statemachine logic.
Complication: chunking; requires list to be changed after its initial transmission. Complication: WrappedHibernateList; not sure how to implement the freeze functionality for those. Will need Martin T's input. Bug revealed: CDOViewImpl.sendDeltaNotifications actually *applies* the deltas, and does so without creating new revisions.
Created attachment 195183 [details] Patch v3
Created attachment 195357 [details] Patch v4 This patch is possibly no different from v3, I just can't remember anymore.
Created attachment 195466 [details] Patch v5 I think I fixed the issues with synthetic revisions (freeze ignored) and the CDONotificationBuilder (list copied before change). Martin agreed to look at the unused warning in WrappedHibernateList after this patch has been committed.
Caspar, feel free to commit patch v5 and resolve this bug.
Committed revision 7692
Reopening: org.eclipse.emf.cdo.tests.config.impl.ConfigTestException: Error in MultiValuedOfAttributeTest.testListOfString [Combined, DBStore: H2 (branching), JVM, Native] at org.eclipse.emf.cdo.tests.config.impl.ConfigTest.runBare(ConfigTest.java:513) at junit.framework.TestResult$1.protect(TestResult.java:110) at junit.framework.TestResult.runProtected(TestResult.java:128) at junit.framework.TestResult.run(TestResult.java:113) at junit.framework.TestCase.run(TestCase.java:124) at org.eclipse.net4j.util.tests.AbstractOMTest.run(AbstractOMTest.java:260) at junit.framework.TestSuite.runTest(TestSuite.java:243) at org.eclipse.emf.cdo.tests.config.impl.ConfigTestSuite$TestWrapper.runTest(ConfigTestSuite.java:126) at junit.framework.TestSuite.run(TestSuite.java:238) at junit.framework.TestSuite.runTest(TestSuite.java:243) at junit.framework.TestSuite.run(TestSuite.java:238) at junit.framework.TestSuite.runTest(TestSuite.java:243) at junit.framework.TestSuite.run(TestSuite.java:238) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197) Caused by: org.eclipse.emf.ecore.resource.impl.ResourceSetImpl$1DiagnosticWrappedException: org.eclipse.emf.cdo.util.InvalidURIException: Invalid URI "cdo://repo1/MultiValuedOfAttributeTest_testListOfString/res1": org.eclipse.net4j.signal.RemoteException: java.lang.IllegalStateException: Cannot modify a frozen list at org.eclipse.emf.ecore.resource.impl.ResourceSetImpl.handleDemandLoadException(ResourceSetImpl.java:315) at org.eclipse.emf.ecore.resource.impl.ResourceSetImpl.demandLoadHelper(ResourceSetImpl.java:274) at org.eclipse.emf.ecore.resource.impl.ResourceSetImpl.getResource(ResourceSetImpl.java:397) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getResource(AbstractCDOView.java:493) at org.eclipse.emf.internal.cdo.view.AbstractCDOView.getResource(AbstractCDOView.java:484) at org.eclipse.emf.cdo.tests.MultiValuedOfAttributeTest.testMultiValuedIOfAttribute(MultiValuedOfAttributeTest.java:175) at org.eclipse.emf.cdo.tests.MultiValuedOfAttributeTest.testListOfString(MultiValuedOfAttributeTest.java:42) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at junit.framework.TestCase.runTest(TestCase.java:168) at org.eclipse.net4j.util.tests.AbstractOMTest.runBare(AbstractOMTest.java:214) at org.eclipse.emf.cdo.tests.config.impl.ConfigTest.runBare(ConfigTest.java:504) ... 18 more Caused by: org.eclipse.emf.cdo.util.InvalidURIException: Invalid URI "cdo://repo1/MultiValuedOfAttributeTest_testListOfString/res1": org.eclipse.net4j.signal.RemoteException: java.lang.IllegalStateException: Cannot modify a frozen list at org.eclipse.emf.internal.cdo.view.AbstractCDOView.registerProxyResource(AbstractCDOView.java:1060) at org.eclipse.emf.cdo.eresource.impl.CDOResourceImpl.registerProxy(CDOResourceImpl.java:901) at org.eclipse.emf.cdo.eresource.impl.CDOResourceImpl.load(CDOResourceImpl.java:825) at org.eclipse.emf.ecore.resource.impl.ResourceSetImpl.demandLoad(ResourceSetImpl.java:255) at org.eclipse.emf.ecore.resource.impl.ResourceSetImpl.demandLoadHelper(ResourceSetImpl.java:270) ... 30 more Caused by: org.eclipse.net4j.signal.RemoteException: java.lang.IllegalStateException: Cannot modify a frozen list at org.eclipse.net4j.signal.RequestWithConfirmation.getRemoteException(RequestWithConfirmation.java:139) at org.eclipse.net4j.signal.RequestWithConfirmation.setRemoteException(RequestWithConfirmation.java:128) at org.eclipse.net4j.signal.SignalProtocol.handleRemoteException(SignalProtocol.java:423) at org.eclipse.net4j.signal.RemoteExceptionIndication.indicating(RemoteExceptionIndication.java:63) at org.eclipse.net4j.signal.Indication.doExtendedInput(Indication.java:55) at org.eclipse.net4j.signal.Signal.doInput(Signal.java:326) at org.eclipse.net4j.signal.Indication.execute(Indication.java:49) at org.eclipse.net4j.signal.Signal.runSync(Signal.java:251) at org.eclipse.net4j.signal.Signal.run(Signal.java:147) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:662) Caused by: java.lang.IllegalStateException: Cannot modify a frozen list at org.eclipse.emf.cdo.internal.common.revision.CDOListImpl.checkFrozen(CDOListImpl.java:109) at org.eclipse.emf.cdo.internal.common.revision.CDOListImpl.set(CDOListImpl.java:165) at org.eclipse.emf.cdo.internal.server.Repository.ensureChunk(Repository.java:616) at org.eclipse.emf.cdo.internal.server.Repository.ensureChunk(Repository.java:537) at org.eclipse.emf.cdo.server.internal.net4j.protocol.LoadChunkIndication.responding(LoadChunkIndication.java:99) at org.eclipse.emf.cdo.server.internal.net4j.protocol.CDOServerIndication.responding(CDOServerIndication.java:133) at org.eclipse.net4j.signal.IndicationWithResponse.doExtendedOutput(IndicationWithResponse.java:96) at org.eclipse.net4j.signal.Signal.doOutput(Signal.java:296) at org.eclipse.net4j.signal.IndicationWithResponse.execute(IndicationWithResponse.java:65) at org.eclipse.emf.cdo.server.internal.net4j.protocol.CDOReadIndication.execute(CDOReadIndication.java:36) ... 5 more
Fix in Repository.ensureChunk(): InternalCDOList cdoList = list instanceof InternalCDOList ? (InternalCDOList)list : null; if (cdoList != null) { cdoList.setWithoutFrozenCheck(startIndex + indexInChunk, id); } else { list.set(startIndex + indexInChunk, id); } Note: Although MEMStore implements a MEMStoreChunkReader it seems that MEMStore can generally not simulate chunking because the *stored* revisions always contain complete lists!
Committed revision 7732
This fix appears to be causing problems when merging an offline branch containing local changes to a multi-valued feature. During the merge, the IDs that were generated for objects added to the feature in offline mode are supposed to be replaced by new CDO temp ids. However, the attempt to set the temp ids in the objects and references fails since the revision being merged is frozen, which causes the merge to fail. I'll try to get a test case working that exposes the problem.
I have additional details on the condition leading to my merge error. In my previous comment, I thought it was related to multi-valued features, but subsequent unit testing doesn't show that. The specific error condition arises when adjusting an EReference to a CDOID that is created while offline. Consider two classes A and B where B augments A and each instance of B holds an EReference to a corresponding unique instance of A (A does not hold a reference to B). While we are in offline mode, we create a new instance of A (aNew) and a new instance of B bNew, setting bNew's a reference to aNew. At creation time, aNew gets a temporary CDOID oid1, and bNew gets oid2. When the offline transaction is committed, these CDOIDs are reassigned to regular CDOIDs, each with a very large long value (e.g. OID9223372036854775806). After we return to an online state and try to merge, we attempt to remap these CDOIDs once again based on the current CDOIDs in the online repository. An IllegalStateException is thrown at the point where we try to remap bNew's reference to aNew. The exception indicates that the CDORevision containing bNew is frozen and cannot be modified. There are numerous exclusions in the CDORevisionImpl.checkFrozen() method that allow CDOID fields to be updated on a frozen revision. However, this case is not excluded because both the old CDOID and the new CDOID are regular (as opposed to temporary) CDOIDs. The relevant exclusion only takes effect if at least the old CDOID is temporary. It seems that a possible fix would involve adding another checkFrozen() exclusion for old CDOIDs that use the large values (noted above) associated with offline commits, and are being replaced with new CDOIDs, but I am not sure if there are other cases where the large CDOIDs are used that would not qualify for the exclusion, so I'm reluctant to make the change blindly. I would appreciate it very much if the experts on this logic can weigh in on this problem with an opinion as to my possible fix. If it seems reasonable, I'd also appreciate advice on how to formulate the test for the offline CDOIDs. In particular, is there a method or a constant value I should use for the comparison? I'll be happy to attach a patch (once it's confirmed) if needed... Thanks, Steve Robenalt
Following the general notion that asking a question is the surest way of finding at least part of the answer, I eventually recognized that the temporary values are based on Long.MAX_VALUE, which indeed is the base for local (i.e. offline mode) CDOIDs. I also found that LongIDStore, LongIDHandler, and ultimately DBStore provide an isLocalCDOID() method allowing me to perform the check. I blame lack of caffeine for missing these before. :-) I'm now trying to decide if it's possible to get legal access to the method from within CDORevisionImpl.checkFrozen() or if there is another way to achieve the same test. For consistency, it seems like having an isLocal() method on CDOID itself would be the best way to perform the test. Next best would be a method on CDOIDUtil, but those don't exist at this point.
I have a temporary workaround for this problem. I don't see any easy way to allow the CDORevisionImpl class to check if a CDOID is local without mucking up the dependencies. As such, I opted for a temporary workaround as follows: Since normal CDOIDs start at 0 (or is it 1?) and grow, and since local CDOIDs start from Long.MAX_VALUE and decrease, the workaround involves creating a new exceptional condition in CDORevisionImpl.checkFrozen(). The specific condition allows for the oldValue to be an instance of CDOIDObjectLongImpl and to have a value greater than Long.MAX_VALUE/2, as well as the check to insure that newValue is an instanceof CDOID, as in exceptional conditions 2a and 2b (per the comments). Since it is unlikely that the number of real CDOIDs or local CDOIDs will ever cross the midpoint of the Long.MAX_VALUE range, this fix suffices for my needs until a more permanent solution is available - and it has been verified to work! I have patches available for one of the offline test cases that exposes the problem in the JUnit tests, and my workaround patch for CDORevisionImpl if they are needed. Steve Robenalt
Steve, I really appreciate your clear exposition of the problem. I agree with your recommended approach: we should be able to identify local ID's as such. I think this will require the introduction of a new ID type, analogous to CDOIDTempObjectImpl. This doesn't strike me as a great deal of work, but then I'm not familiar with the mechanism that issues ID's for new objects on a local branch, and replaces them later when the sync is performed. Eike, can you comment on this? If you agree that this isn't a big job, I think we better aim for the permanent solution instead of committing Steve's workaround. BTW, this will be a non-issue if we use random generated UUID's as we discussed y'day. Do you have a time-frame in mind for this? And will the UUID's become the *only* ID-generation mechanism, or will we maintain the sequential ID mechanism as well? Thanks both.
Hi Steve, We appreciate your engagement very much ;-) The replacement of the local IDs happens here: CDOTransactionImpl.applyLocalIDMapping(CDOChangeSetData) Caspar will write down some more thoughts we've shared...
Available in R20110608-1407
(In reply to comment #17) > [...] Since normal CDOIDs start at 0 (or is it 1?) [...] We must keep in mind that almost all aspects of CDOIDs, especially the assigned values are store-specific.Subclasses of LongIDStore let their IDs start with 1.
Closing
Steve Eike didn't like the idea of adding a new type too much, because it's quite a bit of work. As an alternative, I propose that we add something like #isLocal on the CDOID interface, and add a final field in the implementation(s). Perhaps this can go in AbstractCDOID? Eike has some reservations about this approach as well, but I can't think of any other clean solution.
If we go away a bit from adding the data that's necessary to answer isLocal() on a *client* into each CDOID instance, then there's another solution: We could allow the store to transfer a piece of data/functionality to the client upon OpenSessionRequest. We could use this "object" on the client to ask isLocal(CDOID). More concretely, LongIDStore could (does already?) define a fixed lower limit for local IDs and transmit the actual value of this limit to the client. What about that? BTW. a new field in CDOID affects all the 99.99% of the CDO users that do not use offline and have never heard about local IDs. I suspect it could enlarge each ID instance by 8 bytes. And we have lots of them.
(In reply to comment #24) > BTW. a new field in CDOID affects all the 99.99% of the CDO users that do not > use offline and have never heard about local IDs. I suspect it could enlarge > each ID instance by 8 bytes. And we have lots of them. I hope you're wrong or mistaken about this, because if serializing a single additional boolean adds 8 bytes to the stream, I think we have a huge problem somewhere in our serialization technology :-( > We could allow the store to transfer a piece of data/functionality to the > client upon OpenSessionRequest. We could use this "object" on the client to ask > isLocal(CDOID). > > More concretely, LongIDStore could (does already?) define a fixed lower limit > for local IDs and transmit the actual value of this limit to the client. > > What about that? > Works for me. It's largely identical to Steve's workaround, except for the part where the server tells the client what the limit value is.
(In reply to comment #25) > I hope you're wrong or mistaken about this, because if serializing a > single additional boolean adds 8 bytes to the stream, I think we have > a huge problem somewhere in our serialization technology :-( I'm not talking about the serialization format but the in-memory footprint. I'm just guessing how the different JVMs lay out an additional boolean field (where there's no other boolean field already). > Works for me. It's largely identical to Steve's workaround, except for > the part where the server tells the client what the limit value is. Great. Steve?
(In reply to comment #26) > > Great. Steve? Works for me as well. The only question I'd have pertains to the earlier comment about local ids being store-specific. Would this lead to a case where it would make sense to store an upper and a lower limit? Or can we always assume that local ids would be assigned above the single limit value?
It doesn't necessarily have to be a number at all. A store that uses strings could for example prefix with an "L". Like the CDOID classes the new "local ID detection strategies" must be located in the cdo.common plugin and have no store dependency themselves. Then they can be serialized over the wire. Example: class CDOIDLocalDetectorLongRangeImpl implements CDOIDLocalDetector, Serializeable { private long min, max; public boolean isLocal(CDOID id) {...} }
(In reply to comment #28) > It doesn't necessarily have to be a number at all. A store that uses strings > could for example prefix with an "L". Like the CDOID classes the new "local ID > detection strategies" must be located in the cdo.common plugin and have no > store dependency themselves. Then they can be serialized over the wire. > Example: > > class CDOIDLocalDetectorLongRangeImpl implements CDOIDLocalDetector, > Serializeable > { > private long min, max; > public boolean isLocal(CDOID id) {...} > } Makes sense. I look forward to switching to the official fix...
