Bugzilla – Full Text Bug Listing
| Summary: | Error logged by DsfSuspendTrigger. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Pawel Piech <pawel.1.piech> | ||||||
| Component: | cdt-debug-dsf | Assignee: | Pawel Piech <pawel.1.piech> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | Pawel Piech <pawel.1.piech> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | marc.khouzam, pawel.1.piech | ||||||
| Version: | 7.0 | Flags: | marc.khouzam:
review+
|
||||||
| Target Milestone: | 7.0 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Pawel Piech
Created attachment 157023 [details]
Fix with better error handling.
I committed the fix, which also adds an utility to help manage error codes in request monitors. Marc, please review. I don't understand the fix. The DsfSuspendTrigger error gets triggered when we terminate the backend? Does using the ImmediateExecutor prevent the DsfSuspendTrigger from being called upon termination? Also, the same fix should go into GDBControl_7_0. I can do it, once I understand the fix :-) Thanks (In reply to comment #3) > I don't understand the fix. The DsfSuspendTrigger error gets triggered when we > terminate the backend? Does using the ImmediateExecutor prevent the > DsfSuspendTrigger from being called upon termination? > > Also, the same fix should go into GDBControl_7_0. I can do it, once I > understand the fix :-) > > Thanks I'm sorry, I should have added more information about that change (which is not exactly related to the suspend trigger). Yes I also saw a RejectedExecutionException coming from GDBControl when working on this bug. Using immediate executor gurantees that the rm will be completed even if the session's executor is shut down, and since the request monitor in question is not used to run any other code it's safe to use it. (In reply to comment #4) > I'm sorry, I should have added more information about that change (which is not > exactly related to the suspend trigger). Yes I also saw a > RejectedExecutionException coming from GDBControl when working on this bug. > Using immediate executor gurantees that the rm will be completed even if the > session's executor is shut down, and since the request monitor in question is > not used to run any other code it's safe to use it. I'm ok with this fix, but does it Resolve the bug? Is the following gone? > Request for monitor: 'RequestMonitor > (org.eclipse.cdt.dsf.debug.ui.contexts.DsfSuspendTrigger$4@19fa017): Status > ERROR: org.eclipse.cdt.dsf code=0 null children=[Status ERROR: > org.eclipse.cdt.dsf code=10001 Target not available. null]' resulted in an > error. I also committed this fix to GDBControl_7_0 (In reply to comment #5) > > exactly related to the suspend trigger). Yes I also saw a > > RejectedExecutionException coming from GDBControl when working on this bug. > > Using immediate executor gurantees that the rm will be completed even if the > > session's executor is shut down, and since the request monitor in question is > > not used to run any other code it's safe to use it. > > I'm ok with this fix, but does it Resolve the bug? > Is the following gone? > > > Request for monitor: 'RequestMonitor > > (org.eclipse.cdt.dsf.debug.ui.contexts.DsfSuspendTrigger$4@19fa017): Status > > ERROR: org.eclipse.cdt.dsf code=0 null children=[Status ERROR: > > org.eclipse.cdt.dsf code=10001 Target not available. null]' resulted in an > > error. Sorry Pawel, but before I mark this as reviewed, can you confirm that it fixed the bug you reported? I just find that the solution does not seem to fit the bug description. Thanks Created attachment 157803 [details] Missing changes from DsfSuspendTrigger. (In reply to comment #7) > Sorry Pawel, but before I mark this as reviewed, can you confirm that it fixed > the bug you reported? I just find that the solution does not seem to fit the > bug description. > > Thanks I am sorry. Somehow I managed to leave out the most relevant change out of the patch (and the committed changes). Here is the change to DsfSuspendTrigger, I committed it just now. Sorry for the late review. 1- I think the isSuccess() check at line 89 of DsfSuspendTrigger.java is not necessary. 2- Also, as we saw in bug 301975, it would probably be safer to leave CountingRequestMonitor.setStatus() to check for MultiStatus instead of DsfMultiStatus. I don't think there is value is forcing the check for DsfMultiStatus. I guess the DsfMultiStatus is valuable for its getCode() method, but that one only works because the codes that we use in DSF are all ordered by importance, right? without that, it doesn't make sense. Technically, one could even extend DsfMultiStatus to set the Status.setCode() to this enhanced code, just like MultiStatus does for severity, but that is not essential. Besides points 1 and (maybe) 2, it looks good to me (In reply to comment #9) > Sorry for the late review. > > 1- I think the isSuccess() check at line 89 of DsfSuspendTrigger.java is not > necessary. Good point, I applied the change. > 2- Also, as we saw in bug 301975, it would probably be safer to leave > CountingRequestMonitor.setStatus() to check for MultiStatus instead of > DsfMultiStatus. I don't think there is value is forcing the check for > DsfMultiStatus. I applied this change also. > I guess the DsfMultiStatus is valuable for its getCode() method, but that one > only works because the codes that we use in DSF are all ordered by importance, > right? without that, it doesn't make sense. Yes, I've been trying to figure out how to sensibly use the DSF error codes, and I think this may be the best solution I've found. The ordering by severity is somewhat accidental, but it makes sense. In general, I would expect that clients would only log (or show dialog for) REQUEST_FAILED and INTERNAL_ERROR errors since. INVALID_STATE and INVALID_HANDLE are usually result of expected race conditions. > > Technically, one could even extend DsfMultiStatus to set the Status.setCode() > to this enhanced code, just like MultiStatus does for severity, but that is not > essential. Good point also. I changed the DfeMultiStatus implementation to use this method. Please review again :-) (thank you for the due diligence) (In reply to comment #10) > > Technically, one could even extend DsfMultiStatus to set the Status.setCode() > > to this enhanced code, just like MultiStatus does for severity, but that is not > > essential. > Good point also. I changed the DfeMultiStatus implementation to use this > method. That's even more elegant that I expected :-) |