Fix instances of LCK08-J in UDC

[reprogrammer]

There are two instances of LCK08-J in our instrumentation of UDC:

1. org.eclipse.epp.usagedata.internal.recording.uploading.BasicUploader.transferToCodingSpectator(IProgressMonitor)
2. org.eclipse.epp.usagedata.internal.recording.uploading.UploadManager.preSubmit() and org.eclipse.epp.usagedata.internal.recording.uploading.UploadManager.postSubmit()

We need to release the lock in a finally block. And, we might need to add another method called failedToSubmit to the SumitterListener interface and release the lock in this method.

[vazexqi]

First attempt at f2efab1c5e3ead533509d9b7059296edcd228d71

Please double-check since this is tricky.

To simulate an exception, what I did was the following scenario:

1. Set a breakpoint at Submitter#submit() line 94.
2. Initialize an upload process by going manually to the preference dialog and forcing an upload.
3. Once I hit the breakpoint, I disabled my internet connection. This will create an java.net.UnknownHostException when trying to do the commit.
4. Step through the debugger until you hit svnManager.doCommit() on line 98 in Submitter.
5. You should now be taken to the notifySubmissionException method for each listener.

[reprogrammer]

We have to handle unchecked exceptions as well (See the description of LCK08-J) for more information. Unfortunately, my proposal for adding a method called failedToSubmit doesn't solve the problem elegantly. We could change the code in Submitter.submit to notifySubmissionException in a catch (Trowable t) clause. But, it might be better to rename failedToSubmit to cleanUp and always call it in the finally block of Submitter.submit.

[vazexqi]

Yes that would be a complete solution -- it would also require us to change the documentation for SubmitterListener to document that the interface is meant to be used in a thread-safe way which is why there is the need for cleanup. The initial design of SubmitterListener never made that contract.

It would also require some state be kept in the listeners to check. For instance, we can't just always call lock.unlock() (idempotence), we need to be sure that the lock has actually been held and all that (this is why I used ReentrantLock and check if the lock has been held).

We should check if this is worth the extra trouble as a design decision or whether we should just throw the exception up to Eclipse. These exceptions would probably be very weird exceptions like RuntimeException that cannot be recovered.

[Wanderer777]

It looks that the contract of SubmitterListener.postSubmit is to get a notification that a submit is over regardless of whether it succeeded or failed (and we can add a flag argument to postSubmit, if succeeded/failed result is important, but in the current implementation it does not really matter). This way, instead of adding more methods to SubmitterListener (and the supporting code for them), we could just move notifyPostSubmit call to the finally block of Submitter.submit.

[reprogrammer]

It looks like you're suggesting to merge cleanUp into postSubmit and always invoke postSubmit regardless of whether the submission has succeeded. What if someone is interested in doing something only when the submission has finished successfully? Can you support this behavior in your proposed interface?

[Wanderer777]

Sure, as I wrote above, we can add a flag paremeter to postSubmit method, whose value will tell whether the submission was successful or failed.

[vazexqi]

Let's talk about this more on Monday. There is the issue here of adding scaffolding that only we need in CodingSpectator and that CodingTracker doesn't care about because it does not deal with threads (CodingTracker just needs notifications). By adding more methods, we unnecessarily complicate the contract of SubmitListener for those clients that don't need it.

[vazexqi]

Based on our discussion on Monday, we have added the simplest possible change for the time being to not over-engineer. The contract is as follows:

Before notification, the Submitter sends a preSubmit() notification. This is done outside the try-catch block so it should not be interrupted barring unusual RuntimeExceptions. After the submission, the Submitter sends a postSubmit(boolean) notification in a finally clause. The boolean indicates whether the submission happened successfully or not. Right now no client is actually making use of the boolean value.

These changes are in bb255b95d58e3ce94e5cda4f3501eca8c4316170

[reprogrammer]

Merged back fix-lck08-j into master and closed the issue.