Community
Participate
Working Groups
As I already noted a while ago in a comment at LockObjectsIndication.java, lines 76 ff., it doesn't make much sense that an exception is thrown when the logic in #handleViewedRevision discovers that one of the objects that the client is trying to lock, is not up to date. In this situation, nothing is really "wrong"; it's normal for a client (especially ones with passiveUpdates=false) to hold some stale revisions. One could argue that an exception is simply the easiest way of reporting the problem back to the client, but it has unfortunate side effects. A server-side exception gets wrapped in a RemoteException, sent to the client, and is dumped to the console there, suggesting serious breakage to the user while in fact the problem was to be expected. It's also problematic that the user app can't deal with this expected problem in a straightforward manner. I propose that we rework this to report the problem back through the responding/confirming mechanism.
Sample stacktrace: [ERROR] Client's revision of object OID39 is not the latest version in branch Branch[id=0, name=MAIN] java.lang.IllegalArgumentException: Client's revision of object OID39 is not the latest version in branch Branch[id=0, name=MAIN] at org.eclipse.emf.cdo.server.internal.net4j.protocol.LockObjectsIndication.handleViewedRevision(LockObjectsIndication.java:91) at org.eclipse.emf.cdo.server.internal.net4j.protocol.LockObjectsIndication.indicating(LockObjectsIndication.java:59) at org.eclipse.emf.cdo.server.internal.net4j.protocol.CDOServerIndication.indicating(CDOServerIndication.java:84) at org.eclipse.net4j.signal.IndicationWithResponse.doExtendedInput(IndicationWithResponse.java:90) at org.eclipse.net4j.signal.Signal.doInput(Signal.java:316) at org.eclipse.net4j.signal.IndicationWithResponse.execute(IndicationWithResponse.java:63) at org.eclipse.emf.cdo.server.internal.net4j.protocol.CDOReadIndication.execute(CDOReadIndication.java:36) at org.eclipse.net4j.signal.Signal.runSync(Signal.java:241) at org.eclipse.net4j.signal.Signal.run(Signal.java:147) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(Unknown Source) at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source) at java.lang.Thread.run(Unknown Source)
Caspar, can you work on this? I agree with your prososal.
OK, assigning to myself..
Created attachment 188576 [details] Patch v1
The patch I posted makes the desired changes in the signal classes involved, and that works perfectly fine. But the remaining question is how to work this into the CDOObject API. Consider a call cdoObject.readLock().lock() The return type is currently void. We could keep it that way and translate a failure (which, with my patch in place, is no longer communicated as a RemoteException, but as a normal result through the response stream) back to a local exception, e.g. a 'StaleRevisionEx' or whatever. But a lock could also fail because of a timeout. (This I haven't yet changed to be communicated back as a regular result. Should I? I suppose so.) Or should we have a return type enum LockResult {OK,STALE,TIMEOUT} ? It's a matter of preference really. But the spirit of this bugzilla seems to be that certain reasons for a lock to fail are "normal" and therefore should be communicated as a regular result of the call, rather than as exceptions. I think both stale revisions and timeouts are such "normal" reasons for a lock to fail. Comments anyone?
LockObjectsResult seems to have no effect. LockObjectsResult.isSuccessful is not called from anywhere. Is something missing? Oh, you didn't request a review, yet. Maybe it's work in progress...
(In reply to comment #6) > Oh, you didn't request a review, yet. Maybe it's work in progress... Yeah.. I explained the status of the patch in my my last post of 2011-02-09, and asked for comments...
Maybe I don't see all consequences, yet, but I'd vote for another unchecked exception. My arguments: 1) Part of the CDOLock API is inherited from java.util.concurrent.locks.Lock, where we can not change return types. 2) There is also CDOView.lockObject(...) and it would requre a different return type. 3) Return values are overlooked all too easily
Created attachment 188736 [details] Patch v2 This patch uses the same exceptions that were thrown before, i.e. an TimeoutRuntimeEx for lock timeouts, and an IllegalArgEx for lock failures due to stale revisions. But unlike the original code, the failures are communicated back through the normal response stream instead of using RemoteExceptions and an additional signal. The exceptions are constructed in CDOViewImpl.lockObjects
Created attachment 188737 [details] Patch v3 This patch uses custom exception types.
Created attachment 188747 [details] Patch v4 I've tried to simplify it a little by directly returning the appropriate exception from confirming(). I was just about to approve the patch, when I realized that it breaks the tests. Both patches do that, v3 and v4. The failure is strange, it happens during commit because of revision.setRevised(revision.getTimeStamp()-1). And it only seems to happen if all tests are executed in sequence. Executing the failures only does not fail. Some static state involved??
The failures seem to occur less frequently if executing in debug mode, but they do occur, too.
Darn, the failures also occur without any of the two patches. Just even less frequently. Investigating...
Okay, that was just a timing bug in the RepositoryConfig setup. I fixed it ;-)
Please go ahead and commit, if you don't object to my changes...
Committed to trunk, rev. 7061
Available in R20110608-1407
