Bugzilla – Full Text Bug Listing
| Summary: | [services] Race condition when initializing DSF services | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Khouzam <marc.khouzam> | ||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Marc Khouzam <marc.khouzam> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | cdtdoug, john.cortell, pawel.1.piech | ||||||||
| Version: | 7.0 | Flags: | john.cortell:
review+
|
||||||||
| Target Milestone: | 8.0 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
Yikes! I do remember thinking of this problem when first drafting the initialize/shutdown methods, but I didn't have any good solution and now it's time to pay up :-( At this point, I would suggest to make any initialize() method that registers the service final. Instead make doInitialize() protected and let sub-classes extend that. This is an API-breaking change though. Perhaps we should even make this a pattern in AbstractDsfService in the next release (and up the DSF version). (In reply to comment #0) How about adding an initialize() variant that takes a boolean to indicate whether to register or not. The runtime type will always be called via the standard initialize(). If it's a derivative, it should call it's super via the new initialize method and pass false (which will in turn do the same if it, too, is a derivative). Only the class that had its standard initialize() called will register the service; this will be the leaf-node type (i.e., the runtime type). Moreover, it will be required to register the service since it asked its upstream classes not to do so. If the new method is added by adding an IDsfService2, this could be done without breaking API. All services that support being derived would need to implement the new interface. We'd update all such DSF, DSF-GDB and EDC implementations to do so. I think using the ImmediateExecutor would mean that a service's initialization couldn't hand off any of the work to be performed asynchronously; imagine there needing to be some target access during initialization. (In reply to comment #1) > Yikes! I do remember thinking of this problem when first drafting the > initialize/shutdown methods, but I didn't have any good solution and now it's > time to pay up :-( You're one ahead of me. I was completely blind-sided. Why do these things wait to show up at the end of the release cycle? :-) > At this point, I would suggest to make any initialize() method that registers > the service final. Instead make doInitialize() protected and let sub-classes > extend that. This is an API-breaking change though. Perhaps we should even > make this a pattern in AbstractDsfService in the next release (and up the DSF > version). I don't think I got what you meant. If a service overrides doInitialize() won't it need to call super.doInitialize(new RM()) and then we're back to the same pattern and the same race condition? Here is a bit of clarification on the situation, just in case. As an example, we currently have: GDBProcesses_7_2 extends GDBProcesses_7_1 extends GDBProcesses_7_0 extends AbstractDsfService There has to be a call to register() in GDBProcesses_7_0 since for GDB 7.0, it is the only class that is instantiated. But GDBProcesses_7_2 or GDBProcesses_7_1 could each have a register() also, since they may implement some new interface (they currently don't, but it could happen). Created attachment 192243 [details] Example of John's idea (In reply to comment #2) > (In reply to comment #0) > How about adding an initialize() variant that takes a boolean to indicate > whether to register or not. The runtime type will always be called via the > standard initialize(). If it's a derivative, it should call it's super via the > new initialize method and pass false (which will in turn do the same if it, > too, is a derivative). Only the class that had its standard initialize() called > will register the service; this will be the leaf-node type (i.e., the runtime > type). Moreover, it will be required to register the service since it asked its > upstream classes not to do so. Me like! I attached a patch showing what I think you suggested. Is this right? We don't even need any changes in DSF, although it would be nice to have DSF suggest/enforce this pattern. A minor side-effect is that every derivative of a service will need to call register(). We currently don't do that, but I think it is fine to require it. > If the new method is added by adding an IDsfService2, this could be done > without breaking API. All services that support being derived would need to > implement the new interface. We'd update all such DSF, DSF-GDB and EDC > implementations to do so. I haven't tried that just yet. > I think using the ImmediateExecutor would mean that a service's initialization > couldn't hand off any of the work to be performed asynchronously; imagine there > needing to be some target access during initialization. Now that you made me think about it, I realize that the pattern of passing an RM to initialize() is to support such a case (at least I think it is). If we didn't need that case, we could simply make initialize() a synchronous call. However, using an ImmediateExecutor would not prevent having asynchronous calls to the target during intialize(), it would simply avoid some extra scheduling of the Executor for all other calls. If a call is actually made to the backend, the use of ImmediateExecutor won't affect it, and an extra call to the Executor will be done automatically, which will re-introduce the race condition. So, I now see that using the ImmediateExecutor is not a complete solution. But it would probably effectively avoid running into the issue altogether by reducing the number of time initialize() can be interrupted (in most cases, reducing it to 0 times). It could serve as a (maybe temporary) fallback solution if all else fails (due to technical issues, or API limitations for this release). (In reply to comment #3) > I don't think I got what you meant. If a service overrides doInitialize() > won't it need to call super.doInitialize(new RM()) and then we're back to the > same pattern and the same race condition? The base class initialize() would be responsible for calling register() after doInitialize() is finished. AbstractDsfService: final public void initialize(final RequestMonitor rm) { fTracker = new DsfServicesTracker(getBundleContext(), fSession.getId()); fStartupNumber = fSession.getAndIncrementServiceStartupCounter(); doInitialize( new RequestMonitor(getExecutor(), rm) { @Override protected void handleSuccess() { register(); rm.done(); }}); } But I like John's idea too since it's more backward compatible in that it doesn't force a change in the use of the base initialize() method... i think. Created attachment 192263 [details] Example of Pawel's idea > AbstractDsfService: > > final public void initialize(final RequestMonitor rm) { > fTracker = new DsfServicesTracker(getBundleContext(), > fSession.getId()); > fStartupNumber = fSession.getAndIncrementServiceStartupCounter(); > > doInitialize( > new RequestMonitor(getExecutor(), rm) { > @Override > protected void handleSuccess() { > register(); > rm.done(); > }}); > } I get it now. That is pretty good. To call register() we need the list of names and properties, but we can easily do that with some abstract methods. Also, I think this idea could be enforced by making register() private, or putting an assert to make sure it is only called once. Here is a prototype of the idea. > But I like John's idea too since it's more backward compatible in that it > doesn't force a change in the use of the base initialize() method... i think. Both ideas seem to work, and both seem to be do-able without breaking the API. Enforcing either idea would require breaking the API by making the current AbstractDsfService.initialize() final and maybe more. BTW, I don't think there is a need for IDsfService2. We only need to add the new initialize(boolean, RM) to AbstractDsfService. This makes John's idea even simpler than before. With the two simple prototypes, it is hard to be sure if either idea will hit a scenario that does not work well, so I think we should choose one and try it out completely. I like both ideas but I'm leaning slightly towards Pawel's suggestion for the following reasons: 1- it was easier to understand from the perspective of someone using DSF. When to use initialize(true) vs initialize(false) of John's idea was a bit confusing, although with the example classes, it is not horrible. 2- it enforces more strongly that register() is only called at the very end. Say someone extends a service and forgets to call register() in that derived service; in Pawel's idea, the base classes would take care of things properly; in John's idea, the race condition would be re-introduced. So, my vote goes for Pawel's idea, but by a very thin margin. What do you guys think? (In reply to comment #6) > Enforcing either idea would require breaking the API by making the current > AbstractDsfService.initialize() final and maybe more. If you're doing something local to the one problematic service then you can try either and change our minds later. But maybe we should open a separate bug to fix this in AbstractDsfService in next release? (In reply to comment #6) > Created attachment 192263 [details] > Both ideas seem to work, and both seem to be do-able without breaking the API. > [snip] > BTW, I don't think there is a need for IDsfService2. We only need to add the > new initialize(boolean, RM) to AbstractDsfService. This makes John's idea even > simpler than before. Indeed. We can add methods to the abstract class, but then we'll be introducing the mechanism at the implementation layer rather than at the interface layer. In other words, we'd be adding a mechanism that is specifically for services that derive from AbstractDsfService. However, DSF services are not obligated to derive from that class. At strictly the interface level, there would continue to be nothing to help service implementations sort out who in a service class hierarchy should do the registering of the service. Of course, if we say that all services must derive from AbstractDsfService, then that changes things. > 1- it was easier to understand from the perspective of someone using DSF. When > to use initialize(true) vs initialize(false) of John's idea was a bit > confusing, although with the example classes, it is not horrible. Personally, I think the opposite is true. API that explicitly assigns responsibility for service registration via the method signature seems more intuitive than API that relies on JavaDoc to differentiate between methods that should and ones that shoudn't. Also, I find the xxxx() + doXxxx() pattern confusing in general and try to avoid it when possible. > 2- it enforces more strongly that register() is only called at the very end. > Say someone extends a service and forgets to call register() in that derived > service; in Pawel's idea, the base classes would take care of things properly; > in John's idea, the race condition would be re-introduced. With my approach, if the leaf-node class tells its upstream chain to not register and then forgets to register, the service simply won't be registered and thus won't be available in the debug session, which should lead to an obvious failure or lack of functionality. There would be no race condition to contend with. All that said, I'd be fine with either approach. Just wanted to point out a few things for further consideration. (In reply to comment #7) > (In reply to comment #6) > > Enforcing either idea would require breaking the API by making the current > > AbstractDsfService.initialize() final and maybe more. > > If you're doing something local to the one problematic service then you can try > either and change our minds later. Do you feel it would be safe to only make the change in the one service I saw the problem? On the one hand, I don't know what triggered the issue and am worried that it could present itself at any time; while on the other hand, we didn't see this issue for years, so it may not be likely to happen again. > But maybe we should open a separate bug to > fix this in AbstractDsfService in next release? So let's agree on the was forward for this release. We have two options: 1- Fix the issue for DSF-GDB's IProcesses services only for Indigo. This is simpler and gives us more time to settle on the solution we prefer. The question is, how risky is it to release without fixing this across the board? 2- Fix the issue for all DSF-GDB services. And maybe for EDC also. This is really what we should strive for, but it is pretty late in the release. There are so many things happening with other bug fixes, that I would prefer keeping this change to a minimum for Indigo, so I vote for #1, if you agree it is safe. (In reply to comment #8) > > BTW, I don't think there is a need for IDsfService2. We only need to add the > > new initialize(boolean, RM) to AbstractDsfService. This makes John's idea even > > simpler than before. > > Indeed. We can add methods to the abstract class, but then we'll be introducing > the mechanism at the implementation layer rather than at the interface layer. > In other words, we'd be adding a mechanism that is specifically for services > that derive from AbstractDsfService. However, DSF services are not obligated to > derive from that class. At strictly the interface level, there would continue > to be nothing to help service implementations sort out who in a service class > hierarchy should do the registering of the service. Of course, if we say that > all services must derive from AbstractDsfService, then that changes things. That is a very good point. Being immersed in DSF-GDB, I made the assumption that all DSF service implementation extend AbstractDsfService, but that is not necessarily true. > > 1- it was easier to understand from the perspective of someone using DSF. When > > to use initialize(true) vs initialize(false) of John's idea was a bit > > confusing, although with the example classes, it is not horrible. > > Personally, I think the opposite is true. API that explicitly assigns > responsibility for service registration via the method signature seems more > intuitive than API that relies on JavaDoc to differentiate between methods that > should and ones that shoudn't. Pawel's suggestion would take care of the registering automatically in AbstractDsfService so derived classes would never need to call register() in that case. Looking at the two prototypes I posted, I find that Pawel's solution provided more support by DSF (AbstractDsfService), so required less effort from extenders. That is really what I liked. Maybe I didn't see that in the other prototype because I didn't make the changes in DSF directly... > Also, I find the xxxx() + doXxxx() pattern > confusing in general and try to avoid it when possible. Agreed, but I believe this pattern will remain in both solutions (unless my prototype didn't illustrate your idea properly). > > 2- it enforces more strongly that register() is only called at the very end. > > Say someone extends a service and forgets to call register() in that derived > > service; in Pawel's idea, the base classes would take care of things properly; > > in John's idea, the race condition would be re-introduced. > > With my approach, if the leaf-node class tells its upstream chain to not > register and then forgets to register, the service simply won't be registered > and thus won't be available in the debug session, which should lead to an > obvious failure or lack of functionality. There would be no race condition to > contend with. That is true. But I was thinking of a person extending a service and thinking that the base class calling register() is just fine for their needs and writing something like: @Override protected void initialize(final boolean register, final RequestMonitor rm) { // tell the base class to do the register for us. We don't need to change, // anything for that call. super.initialize(TRUE, new RequestMonitor(getExecutor(), rm) { @Override protected void handleSuccess() { doInitialize(register, rm); } }); } private void doInitialize(boolean register, final RequestMonitor rm) { // Oh oh, the service is already registered! // Get the services references ... } > All that said, I'd be fine with either approach. Just wanted to point out a few > things for further consideration. Same here :-) If we postpone the full solution for the next release, it can give us the opportunity to change our minds. (In reply to comment #10) > Pawel's suggestion would take care of the registering automatically in > AbstractDsfService so derived classes would never need to call register() in > that case. Looking at the two prototypes I posted, I find that Pawel's > solution provided more support by DSF (AbstractDsfService), so required less > effort from extenders. That is really what I liked. Maybe I didn't see that > in the other prototype because I didn't make the changes in DSF directly... I, too, like that aspect of Pawel's proposal. However, unless we make AbstractDsfService#register() private, it all seems a bit hazardous. Basically, if we're OK with churning the API, I prefer Pawel's approach. If we want to avoid breaking APIs, I prefer mine. In the end, the primary objective of my solution was to avoid breaking API. > > Also, I find the xxxx() + doXxxx() pattern > > confusing in general and try to avoid it when possible. > > Agreed, but I believe this pattern will remain in both solutions (unless my > prototype didn't illustrate your idea properly). Unless I'm overlooking something, doInitialize() in the prototype (of my idea) is an extraneous abstraction. Its logic could very well be placed in the RM's handleSuccess(). > That is true. But I was thinking of a person extending a service and thinking > that the base class calling register() is just fine for their needs and writing > something like: Good point. We could mitigate that pitfall through JavaDoc on that parameter ("derivatives calling this method should pass false and do the registration themselves to avoid having the service registered and used before the derivative has a chance to initialize the service"). However, once we start relying on JavaDoc to prevent client misuse, my approach starts losing part of the edge I think it has. > If we postpone the full solution for the next release, it can give us the > opportunity to change our minds. I'm fine with that. I think using ImmediateExecutor is a suitable workaround until after Indigo. (In reply to comment #11) > I, too, like that aspect of Pawel's proposal. However, unless we make > AbstractDsfService#register() private, it all seems a bit hazardous. Agreed. The assert would help, but it is not full-proof. > if we're OK with churning the API, I prefer Pawel's approach. If we want to > avoid breaking APIs, I prefer mine. In the end, the primary objective of my > solution was to avoid breaking API. Ok, I see your motivation more clearly now. > > > Also, I find the xxxx() + doXxxx() pattern > > > confusing in general and try to avoid it when possible. > > > > Agreed, but I believe this pattern will remain in both solutions (unless my > > prototype didn't illustrate your idea properly). > > Unless I'm overlooking something, doInitialize() in the prototype (of my idea) > is an extraneous abstraction. Its logic could very well be placed in the RM's > handleSuccess(). That is true, but that is also the case for the current way we use initialize() and doInitialize(), and yet we have that pattern, so I hadn't considered that as an added value. So, in the prototype for Pawel's idea, once we can make initialize() final, then implementor will only override performInitialization(). But you are right that the pattern of initialize() and performInitialization() becomes a necessity in this solution. > > That is true. But I was thinking of a person extending a service and thinking > > that the base class calling register() is just fine for their needs and writing > > something like: > > Good point. We could mitigate that pitfall through JavaDoc on that parameter > ("derivatives calling this method should pass false and do the registration > themselves to avoid having the service registered and used before the > derivative has a chance to initialize the service"). However, once we start > relying on JavaDoc to prevent client misuse, my approach starts losing part of > the edge I think it has. I also think that relying on javadoc only should be avoided if we can. > > If we postpone the full solution for the next release, it can give us the > > opportunity to change our minds. > > I'm fine with that. I think using ImmediateExecutor is a suitable workaround > until after Indigo. I was hoping someone would say that :-) I'm also strongly leaning towards that for this release. Then in the next release, we change the APIs we need to change and we get a solid solution. Let me ask your opinion again, as you usually have a good sense for such things, do you think I should go for the ImmediateExecutor in every instance of this pattern, or just in the case I noticed the problem. I was planning on going across the board with it (DSF-GDB only). Pawel, are you ok with the ImmediateExecutor solution for Indigo and a bug on DSF to change AbstractDsfService in the next release? (In reply to comment #12) > Let me ask your opinion again, as you usually have a good sense for such > things, do you think I should go for the ImmediateExecutor in every instance of > this pattern, or just in the case I noticed the problem. I was planning on > going across the board with it (DSF-GDB only). +1 for using ImmediateExecutor for all hierarchical services in DSF-GDB. Like you said, it will probably completely remove the race condition (as long as clients extending the DSF-GDB services also use ImmediateExecutor), and worst case, it will reduce the window size. Created attachment 192376 [details] Using ImmediateExecutor in DSF-GDB Here is the patch that uses the ImmediateExecutor. For the CommandControl service and the Backend service, we still use the real executor form part of the initialize() because we rely on the Sequence class. This solution is not perfect but it should be good enough for this release. I have opened Bug 341660 to get proper DSF support to fix this right, for the next release. I committed this patch to HEAD. Thanks John and Pawel for your quick feedback and for your suggestions. John, can you review this patch? *** cdt cvs genie on behalf of mkhouzam *** Bug 341423: Race condition when initializing DSF services. Temporary solution for the Indigo release. [*] GDBProcesses_7_2.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_2.java?root=Tools_Project&r1=1.6&r2=1.7 [*] GDBProcesses_7_1.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_1.java?root=Tools_Project&r1=1.3&r2=1.4 [*] GDBProcesses.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses.java?root=Tools_Project&r1=1.21&r2=1.22 [*] GDBTraceControl_7_2.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBTraceControl_7_2.java?root=Tools_Project&r1=1.3&r2=1.4 [*] GDBProcesses_7_0.java 1.43 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_0.java?root=Tools_Project&r1=1.42&r2=1.43 [*] GDBBackend.java 1.23 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java?root=Tools_Project&r1=1.22&r2=1.23 [*] GDBBreakpoints_7_0.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBreakpoints_7_0.java?root=Tools_Project&r1=1.9&r2=1.10 [*] GDBRunControl.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBRunControl.java?root=Tools_Project&r1=1.18&r2=1.19 [*] GDBMemory_7_0.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBMemory_7_0.java?root=Tools_Project&r1=1.4&r2=1.5 [*] GDBRunControl_7_0.java 1.27 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBRunControl_7_0.java?root=Tools_Project&r1=1.26&r2=1.27 [*] GDBRunControl_7_0_NS.java 1.31 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBRunControl_7_0_NS.java?root=Tools_Project&r1=1.30&r2=1.31 [*] MacOSGDBRunControl.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/macos/MacOSGDBRunControl.java?root=Tools_Project&r1=1.4&r2=1.5 [*] MacOSGDBProcesses.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/macos/MacOSGDBProcesses.java?root=Tools_Project&r1=1.2&r2=1.3 [*] GDBControl_7_0.java 1.31 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl_7_0.java?root=Tools_Project&r1=1.30&r2=1.31 [*] GDBControl.java 1.27 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl.java?root=Tools_Project&r1=1.26&r2=1.27 [*] MIStack.java 1.19 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIStack.java?root=Tools_Project&r1=1.18&r2=1.19 [*] CSourceLookup.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/CSourceLookup.java?root=Tools_Project&r1=1.6&r2=1.7 [*] MIBreakpointsManager.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpointsManager.java?root=Tools_Project&r1=1.21&r2=1.22 [*] MIProcesses.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIProcesses.java?root=Tools_Project&r1=1.11&r2=1.12 [*] MIRunControl.java 1.34 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIRunControl.java?root=Tools_Project&r1=1.33&r2=1.34 [*] MIExpressions.java 1.23 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIExpressions.java?root=Tools_Project&r1=1.22&r2=1.23 [*] MIMemory.java 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIMemory.java?root=Tools_Project&r1=1.7&r2=1.8 [*] MIBreakpoints.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpoints.java?root=Tools_Project&r1=1.17&r2=1.18 [*] MIRegisters.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIRegisters.java?root=Tools_Project&r1=1.9&r2=1.10 [*] MIModules.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIModules.java?root=Tools_Project&r1=1.3&r2=1.4 [*] MIDisassembly.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIDisassembly.java?root=Tools_Project&r1=1.4&r2=1.5 I did only a spot check on the changes since there's lots of them and the change is just a one line change to use the ImmediateExecutor instead of the dsf session's executor. |
I started seeing a DSF service being used before it has been initialized! Normally, this would not happen because we register the service with OSGI (so it can be found by its users) during initialization. Therefore, to be able to find a service and use it, that service has to be initialized. However, I now realize it is possible for a service to be called in the middle of its initialization, after it has registered but before the initialization is finished! Each DSF service must implement IDsfService.initialize(). The pattern that we have been using is: @Override public void initialize(final RequestMonitor requestMonitor) { super.initialize(new RequestMonitor(getExecutor(), requestMonitor) { @Override protected void handleSuccess() { doInitialize(requestMonitor); } }); } When having a hierarchy of service classes we end up call initialize() for the highest parent and then dropping down from children to children calling doInitialize(). The problem is that some service register themselves in a parent class and therefore make themselves available before all the children classes are initialized. Because we use the DSF executor, the initialization() can actually be interleaved with other executor calls, which in some cases, start using the service. A long time ago, we would only register a service in the last child class. This broke down once other people started extending services because the base class in DSF-GDB was still doing a register. So, we had to allow to register multiple times. The solution I see for this problem is to use the ImmediateExecutor in the super.initialiaze() pattern above. This would make the entire initialization() one continuous Executor operation that could not be interrupted. I don't know if we could enforce that beyond putting some javadoc in IDsfService. Any other opinions?