Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 325943

Summary: Robustify Sequence against RejectedExecutionException
Product: [Tools] CDT Reporter: Vladimir Prus <vladimir.prus>
Component: cdt-debug-dsfAssignee: Project Inbox <cdt-debug-dsf-inbox>
Status: RESOLVED FIXED QA Contact: Pawel Piech <pawel.1.piech>
Severity: normal    
Priority: P3 CC: marc.khouzam, nobody, pawel.1.piech
Version: 8.0Flags: marc.khouzam: review?
Target Milestone: 8.0   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Initial patch pawel.1.piech: iplog+

Description Vladimir Prus CLA 2010-09-22 06:33:53 EDT 
Build Identifier: 

Sequence.executeStep uses a RequestMonitor instance whose done method is called when the step is done. The implementation of RequestMonitor.done() calls handleCompleted via executor. However, sometimes the executor may be gone. In that case, RejectedExecutionException is raised, and RequestMonitor.handleRejectedExecution is called. That method calls done on the parent monitor. For Sequence, the parent monitor is RequestMonitorWithProgress whose 'handleCompleted' method does not do much. 

As result, the 'finish' method of Sequence is never called, and any code blocked in Sequence.get remains blocked forever.

Attached patch fixes this in most straightforward way. It might be better to notify about the error, but it's not obvious to me how.

Reproducible: Sometimes

Steps to Reproduce:
This reproduces in our product that implements run via start-debug-session-detach-terminate sequence.
Comment 1 Vladimir Prus CLA 2010-09-22 06:36:10 EDT 
Created attachment 179365 [details]
Initial patch
Comment 2 Vladimir Prus CLA 2010-09-22 06:36:30 EDT 
List discussion at http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg20416.html
Comment 3 Pawel Piech CLA 2010-09-27 19:36:48 EDT 
> How do you suggest to handle errors? IIUC, for the case of final launch
> sequence, Sequence.fRequestMonitor is not actually used by the clients,
> so even if we call setStatus on it, nobody will notice.

I checked in a fix that causes the Sequence.get() call to throw an exception.  This exception will need to be handled in the launch delegate.

Please let me know if this fix works for you.

Mikhail, please review when you get a chance.
Comment 4 Marc Khouzam CLA 2010-09-28 10:12:11 EDT 
(In reply to comment #3)
> I checked in a fix that causes the Sequence.get() call to throw an exception. 
> This exception will need to be handled in the launch delegate.

Pawel, you no longer attach patches? :-)
Comment 6 Nobody - feel free to take it CLA 2010-09-28 12:56:16 EDT 
Looks good to me.
Comment 7 Pawel Piech CLA 2010-09-28 13:40:48 EDT 
(In reply to comment #4)
> Pawel, you no longer attach patches? :-)
I thought that with CDT genie we no longer need to, right?
Comment 8 Marc Khouzam CLA 2010-09-28 15:09:00 EDT 
(In reply to comment #7)
> (In reply to comment #4)
> > Pawel, you no longer attach patches? :-)
> I thought that with CDT genie we no longer need to, right?

That was a gray area for me.  The CDT Genie sometimes takes a while to kick in (I've seen some instances of it taking weeks, although maybe that was at the beginning).

Also, one cannot revert a patch unless we have it attached.  And we cannot apply it another branch, if needed.  The bugzilla patch diff is also nice to use, when a patch is attached.

As you can see, I really like it when patches are attached.  But that is just my own preference.
Comment 9 Vladimir Prus CLA 2010-10-02 09:04:24 EDT 
FWIW, I agree with Marc. The auto-added changes have a few problems:

0. CDT uses CVS
1. There's one link per file, which is inconvenient for patches that touch more than one file.
2. Getting individual patch requires quite some clicks.
3. Each patch only has basename of the file in the header, which makes applying hard.

So, it would be nice that either CDT switch to a version control system that has atomic commits, or that patches be attached to bugzilla.