Bugzilla – Full Text Bug Listing
| Summary: | [Pin&Clone] Pinned view should blank out when session terminated | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Patrick Chuong <pchuong> | ||||||||||||||
| Component: | cdt-debug | Assignee: | Patrick Chuong <pchuong> | ||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Ken Ryall <ken.ryall> | ||||||||||||||
| Severity: | normal | ||||||||||||||||
| Priority: | P3 | CC: | cdtdoug, marc.khouzam, pawel.1.piech | ||||||||||||||
| Version: | 8.0 | Flags: | marc.khouzam:
review+
|
||||||||||||||
| Target Milestone: | 8.0 | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Windows XP | ||||||||||||||||
| Whiteboard: | |||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 337376 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Patrick Chuong
Created attachment 190000 [details]
patch
This patch listens to ILaunchesListener2#launchesTerminated() and fire an empty selection change event.
Currently, what happens if we pin to a thread and the thread disappears (completes)? I'm curious if we handle this well and if we do, maybe we can re-use the same approach for this bug? It doesn't do anything, the view keeps the data and if the thread is alive again, the view should auto reattach to the thread. The current pin implementation doesn't know when a thread is gone or the target is not accessible, the backend will need to generate an update delta if this is the desire behavior. Or we could introduce a callback interface for the backend to notify the pin provider to generate an empty selection event. However, I don't like this approach, because there isn't any selection really happened. The thread terminated issue is similar to the disassembly issue (337376), this also can be reproduce with any view, not just the disassembly view. I think the proper way to address blanking out the view when thread is terminated or not accessible should be handle by the backend with model change delta. What do you think? (In reply to comment #3) > It doesn't do anything, the view keeps the data and if the thread is alive > again, the view should auto reattach to the thread. The current pin > implementation doesn't know when a thread is gone or the target is not > accessible, the backend will need to generate an update delta if this is the > desire behavior. Or we could introduce a callback interface for the backend to > notify the pin provider to generate an empty selection event. However, I don't > like this approach, because there isn't any selection really happened. > > The thread terminated issue is similar to the disassembly issue (337376), this > also can be reproduce with any view, not just the disassembly view. I think the > proper way to address blanking out the view when thread is terminated or not > accessible should be handle by the backend with model change delta. What do you > think? It makes sense I think. Can't we use the same approach for a terminated session? Marc, instead of making changes to each VMProvider, I introduced a callback interface for the GDBPinProvider to notify the DebugEventfilterService to generate a dummy empty selection event when the context exited or resumed. Can you take a look at this patch and let me know whether this is the right thing to do? Created attachment 190289 [details]
Patch with callback interface
(In reply to comment #6) > Created attachment 190289 [details] > Patch with callback interface I like this approach as it keeps the changes to Pin&Clone classes. I'm going over the patch now. Is it ok to send a bogus "new StructuredSelection()" as a current context, when the callback is called? I guess it may seem ok when we want to blank the view, but if the model changed for another reason, we probably want to properly update, no? Shouldn't we send the last context we received? P.S. To go faster, instead of comments, I'm modifying the patch directly, and I'll post the new patch. You'll be able to do a diff with your previous patch. But I'd like to know what you think of the above first. This change seems to break things:
+ Display.getDefault().asyncExec(new Runnable() {
+ public void run() {
+ fActiveContext = event.getContext();
+ fire(event);
+ }
1- run a program with two threads
2- pin variables view to one thread
3- select stack frame of second thread (view does not change, good)
4- unpin variables view
-> view still does not change and shows old data.
This used to work and when I remove the change above, it works again. Any ideas?
Created attachment 190318 [details]
Patrick's solution with a little cleanup
I updated Patrick's last patch to clean up a bit. I also fixed some javadoc typos. The important changes are:
1- I moved the new IPinModelListener to IPinProvider, since that was approach for the other interfaces related to Pinning
2- I created the IPinModelListener directly in DebugContextPinProvider, which removed the circular pointing between DebugContextPinProvider and DebugEventFilter. In fact, DebugEventFilterService no longer needs any change.
3- I removed the use of Display.getDefault().asyncExec(), because of the problem I reported, although I don't understand if we need it or not.
Patrick, please let me know if you disagree with any of the changes that I made.
This patch still uses "new StructuredSelection()" to clear the view. When I tried to use 'fActiveContext', it didn't work. I don't know why. But I'm still concerned about using a bogus selection like that.
Created attachment 190321 [details]
Update Marc's patch
Marc, we need the delegateEvent to fire in the UI thread. There are other pieces of code that assume the debug context change event is from the UI thread.
I initially put in into an asyncExec to prevent a deadlock from the GdbConnectCommand.query(), and introduced the problem that you. I have change it to syncExec, it should resolved the unpin issue. The reason that asynExec doesn't work is due to the view listener is unregistered when asynExec is executed.
Also, you have removed the IContainerDMContext check in GDBPinProvider, this is necessary for gdb6.8, which is what I am using. I guess you are using other gdb version, which executed the IExecutionDMContext check.
Regarding reuse fActiveContext in the selection event, this is the issue that I raised earlier about inconsistence; the view might not blank out.
You can diff it with your modified patch and see the changes.
(In reply to comment #11) > Created attachment 190321 [details] > Update Marc's patch > > Marc, we need the delegateEvent to fire in the UI thread. There are other > pieces of code that assume the debug context change event is from the UI > thread. > > I initially put in into an asyncExec to prevent a deadlock from the > GdbConnectCommand.query(), and introduced the problem that you. I have change > it to syncExec, it should resolved the unpin issue. The reason that asynExec > doesn't work is due to the view listener is unregistered when asynExec is > executed. Ok, that makes sense. Thanks > Also, you have removed the IContainerDMContext check in GDBPinProvider, this is > necessary for gdb6.8, which is what I am using. I guess you are using other gdb > version, which executed the IExecutionDMContext check. In all versions of GDB, a container is an IExecutionDMC. What is the scenario that you need this for? > Regarding reuse fActiveContext in the selection event, this is the issue that I > raised earlier about inconsistence; the view might not blank out. Which issue was that again? Did you mention that issue in this bug? > You can diff it with your modified patch and see the changes. Will do, first thing tomorrow. I am running gdb on windows, when I terminate the launch or resume the process, I don't hit the IExecutionDMC condition. The event's DMC doesn't equals the pinned Thread DMC, I have to query for the IContainerDMContext to get the equals to return true. I think I mentioned the inconsistence in email. I have observe this in our debugger, I don't remember whether I see this in gdb or not. Created attachment 190398 [details] Update for thread vs process events (In reply to comment #11) In the new patch, you use a Job in GdbPinProvider to update the model? Why is that needed? > I initially put in into an asyncExec to prevent a deadlock from the > GdbConnectCommand.query(), and introduced the problem that you. I have change > it to syncExec, So, are we at risk of a deadlock with GdbConnectCommand.query()? When did you see the problem? We may need to update GdbConnectCommand somehow. (In reply to comment #13) > I am running gdb on windows, when I terminate the launch or resume the process, > I don't hit the IExecutionDMC condition. The event's DMC doesn't equals the > pinned Thread DMC, I have to query for the IContainerDMContext to get the > equals to return true. I see now, I was running non-stop and you are running all-stop. In all-stop, we get a ContainerResumed/Suspended event instead of individual thread ones. The problem about your approach is that in non-stop, if I'm pinned to thread 1 and I resume thread 2, they have the same container and the pinned view will clear, even though thread 1 did not resume. I have modified the patch to fix that by using IMIExecutionDMC instead and by checking for null to know if we should look at the thread or the process. Does the logic makes sense to you? I'm ok with this version of the patch if you are. I have to use a Job inside fireModelChangeEvent to free the dsf executor thread. The reason for this is there are gdb commands that uses the query.get() method in the UI thread, and if I do syncExec within the dsf executor thread, then I'll get a deadlock. Yes, we should look at the usage of query for the commands and see if it sense to add a time out or not. To reproduce the deadlock, you can fire the model change event without the job and do single steps. I see deadlock in GdbConnectCommand and one other command (don't remember the name). I am ok with the changes in doHandleEvent. I am going to commit this patch. Committed 'Update for thread vs process events' patch to HEAD. *** cdt cvs genie on behalf of pchuong *** Bug 338472 - [Pin&Clone] Pinned view should blank out when session terminated [*] IPinProvider.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/ui/IPinProvider.java?root=Tools_Project&r1=1.2&r2=1.3 [*] ViewIDCounterManager.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/pinclone/ViewIDCounterManager.java?root=Tools_Project&r1=1.1&r2=1.2 [*] DebugContextPinProvider.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/pinclone/DebugContextPinProvider.java?root=Tools_Project&r1=1.2&r2=1.3 [*] GdbPinProvider.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/GdbPinProvider.java?root=Tools_Project&r1=1.3&r2=1.4 Created attachment 190422 [details]
Fix for race condition with GDB 6.8
I noticed a race condition with GDB 6.8 when terminating the launch.
The thread events of GDB 6.8 are not as reliable as for GDB > 7.0 because they are not triggered by GDB. I noticed that when terminating the launch, we sometimes do not get any IExited event. If you pin a view and terminate the launch, and repeat that test multiple times, eventually you'll hit an occurrence when the view will not clear.
This patch adds a listener for the ICommandControlShutdownDMEvent, which indicates a shutdown.
Patrick, what do you think?
(In reply to comment #18) > Patrick, what do you think? Looks OK to me. Can you commit the change? thanks. (In reply to comment #19) > (In reply to comment #18) > > Patrick, what do you think? > > Looks OK to me. Can you commit the change? thanks. Commited to HEAD. *** cdt cvs genie on behalf of mkhouzam *** Bug 338472: Pinned view should blank out when session terminated [*] GdbPinProvider.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/GdbPinProvider.java?root=Tools_Project&r1=1.4&r2=1.5 |