Bugzilla – Full Text Bug Listing
| Summary: | InterruptedException being logged by ResourceSetWorkbenchEditSynchronizer | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP Common Tools | Reporter: | Aidyl Kareh <amkareh> | ||||||||||
| Component: | wst.common | Assignee: | Aidyl Kareh <amkareh> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Carl Anderson <ccc> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | amkareh, david_williams, jsholl, malbedaiwi | ||||||||||
| Version: | 3.2 | Flags: | ccc:
review+
|
||||||||||
| Target Milestone: | 3.2.2 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 169217 [details]
Proposed Patch
Currently we are just logging the exception and nothing else is done. The proposed patch would propagate the interrupt to upstream code instead of logging it.
Created attachment 169255 [details]
Do the processing first, then propogate the interrupt
moving out to 320P moving to 3.2.1 The propagation of the interrupt should not cause additional problems. I approve of this fix. * Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such.
An adopter product has been showing this exception in its logs. This exception is currently just being logged. The correct behavior would be to catch the InterruptedException and preserve evidence that the interruption occurred so that code higher up on the call stack can learn of the interruption and respond to it if it wants to.
* Is there a work-around? If so, why do you believe the work-around is
insufficient?
No.
* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?
Was unable to reproduce exception on demand. Code review only.
* Give a brief technical overview. Who has reviewed this fix?
This patch catches the InterruptedException received while trying to acquire a lock and changes the thread's interrupt status so that upstream code can react to the interruption if necessary. Carl and Min Idzelis reviewed this patch.
* What is the risk associated with this fix?
I believe there is minimal risk since the exception is not being thrown up the stack. Only the thread's interrupt status is changed so code that wants to react, can react.
Isn't there a guideline that says you shouldn't throw an exception from a finally block? I'd have to check. Can you explain a little more what cause the exception, and what clients might do with it? Is there a "real" case that needs to do something now? Or is that hypothetical? In other words, is this a "real" immediate, functional need ... or, just better logging? I'm not too concerned, but seems like throwing an Exception where one was not previously thrown is a "change in behavior". So, would appreciate a little more explanation before approving. Does this impact API? Would its JavaDoc have to be changed?
I think the "isInterrupted" processing might even be in the wrong "finally" clause? If I'm looking at it right, once applied, the code basically has
try {...} catch (InterruptedException e) {set variable}
try {... some other stuff ...} finally { handle variable }
So ... question is ... why do that "other stuff" if the Interrupted Exception was thrown?
[The patch applied cleanly, but guess I could be looking at it wrong?]
I think this one deserves some more thought and justification, if you don't mind. I'd be glad to change my review vote, if its worse than it sounds ... or if the code really is supposed to work this way (with "some processing" happening even after the exception is caught).
Thanks.
= = =
BTW, I was looking at this so closely because of throwing and exception in finally ... not sure that taboo applies here, exactly ... but did find some references that recommended against doing that (or, even, calling code in a finally that might throw an exception) since that can cause you to lose any information about the original exception that happened before the finally occurred. So, that's why I'm not sure it applies, since you got an IE, and just throwing another one. But ... still seems kind of risky. I suspect there's no reason for that check to be in a "finally" clause?
Moving out to 3.2.2 *** Bug 320311 has been marked as a duplicate of this bug. *** Created attachment 175095 [details]
Proposed Patch
Hi David, I attached an updated version of the patch which takes into consideration the comments you made on how it was implemented. As far as a reason for the fix, there is no immediate, functional need. This exception was found by an adopter product and rather than just removing the log ( catch(InterruptedException e) {//do nothing}) I thought that the correct behavior would be to catch the InterruptedException and preserve evidence that the interruption occurred so that (theoretically speaking) code higher up on the call stack can learn of the interruption and respond to it if it wants to.
Aidyl, Carl and I were just looking at this to see where this locking mechanism was introduced, and it came in with bug 155737 in version 1.13. Based on all the work that was recently done on ProjectResourceSetImpl with respect to synchronization, we think the best course of action is to actually back out the change that initially introduced the need for this locking. Please try backing out the 1.13 changes and run some regression testing on it including the junit bucket. Clearing the flags for WTP 3.2.2 Created attachment 175931 [details] New Proposed Patch This patch backs out all of the changes made through bug 155737 (version 1.13) which introduced the locking mechanism. The Java EE Tools JUnit bucket was ran against this patch, and it ran successfully. I approve of this fix. Committed to HEAD for WTP 3.2.2 and WTP 3.3 |
Build Identifier: WTP 3.2 RC1 I saw the following InterruptedException in my logs: java.lang.InterruptedException at org.eclipse.core.internal.jobs.OrderedLock.acquire(OrderedLock.java:95) at org.eclipse.wst.common.internal.emfworkbench.integration.ResourceSetWorkbenchEditSynchronizer.aquireLock(ResourceSetWorkbenchEditSynchronizer.java:196) at org.eclipse.wst.common.internal.emfworkbench.integration.ResourceSetWorkbenchEditSynchronizer.acceptDelta(ResourceSetWorkbenchEditSynchronizer.java:203) at org.eclipse.wst.common.internal.emfworkbench.integration.ResourceSetWorkbenchEditSynchronizer.resourceChanged(ResourceSetWorkbenchEditSynchronizer.java:141) at org.eclipse.core.internal.events.NotificationManager$2.run(NotificationManager.java:291) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.core.internal.events.NotificationManager.notify(NotificationManager.java:285) at org.eclipse.core.internal.events.NotificationManager.broadcastChanges(NotificationManager.java:149) at org.eclipse.core.internal.resources.Workspace.broadcastPostChange(Workspace.java:327) at org.eclipse.core.internal.resources.Workspace.endOperation(Workspace.java:1181) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1984) Reproducible: Always