Bugzilla – Full Text Bug Listing
| Summary: | Saving changes to launch configuration should not happen concurrently on tear-down | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | James Blackburn <jamesblackburn+eclipse> | ||||||
| Component: | cdt-debug-cdi-gdb | Assignee: | Project Inbox <cdt-debug-cdi-gdb-inbox> | ||||||
| Status: | RESOLVED WONTFIX | QA Contact: | Elena Laskavaia <elaskavaia.cdt> | ||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | nobody, pawel.1.piech | ||||||
| Version: | 7.0 | Flags: | nobody:
review+
|
||||||
| Target Milestone: | --- | ||||||||
| Hardware: | PC | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
James Blackburn
Created attachment 170306 [details]
patch 2
Fix incorrectl removal of synchronized on CSettingsManager#save()
Mikhail, I think this is sufficient to fix this issue, and is reasonably low risk. We can defer this post 7 depending on what you think. review James, the patch would probably work in this case. But there are some potential problems with it: 1. LaunchConfigurationWorkingCopy.setAttribute() calls setDirty() which notifies all registered launch configuration listeners of changes. It's not a good practice to do it from a synchronized method (I am guilty of having done it before). 2. There is no guarantee that ILaunch always returns the same launch configuration object. The current implementation of ILaunch does return the same launch configuration object, but if for some reason the platform folks decide to change the implementation (which is very unlikely) it will break the synchronization. Thanks Mikhail, makes sense. I guess we should synchronize the managers' save() in some other way - perhaps there'll have to be a separate lock for this, or a static lock in the Manager base class? The race is in accessing the launch configs, so it seems a shame to have a lock different / removed from that... (I also noticed that, as well as launch configs being shared, if you fire off a launch while another is launching, the launch delegate instance is shared, leading to all sorts of problems if there's instance state lying around!) (In reply to comment #5) I think a separate lock would be fine. I am more concern about the second issue, but don't know how to avoid it. There is no way to separate the firing of notifications from the attribute settings. (In reply to comment #6) > (In reply to comment #5) > I am more concern about the second issue, but don't know how to avoid it. There > is no way to separate the firing of notifications from the attribute settings. Ignore this part. Using a separate lock would solve the problem. Comment on attachment 170306 [details]
patch 2
When I get another chance to look at this, I think I'll fix this another way...
The managers don't extend a common base class. But more importantly, it's problematic for us that user private settings (which don't affect the launch directly) such as: memory monitor addresse & the visible global variables are persisted here. We share launches in source control repositories, and its very painful for users to reconcile these changes.
Not planning to work on this in the immediate future. (this is part of a batch change) The CDI debug implementation has been removed in CDT 9.0. Please see bug 484900 and the entry on the New and Noteworthy page https://wiki.eclipse.org/CDT/User/NewIn90#API_modifications |