Bug 341660 - [services] Support to avoid race condition when initializing DSF services
Summary: [services] Support to avoid race condition when initializing DSF services
Status: NEW
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf (show other bugs)
Version: 7.0   Edit
Hardware: PC Linux
: P3 major (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact: Jonah Graham CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks: 341465
  Show dependency tree
 
Reported: 2011-04-01 14:11 EDT by Marc Khouzam CLA
Modified: 2020-09-04 15:20 EDT (History)
3 users (show)

See Also:


Attachments
Proposed fix (Pawel's solution) (14.05 KB, patch)
2011-04-01 15:51 EDT, Marc Khouzam CLA
marc.khouzam: iplog-
Details | Diff
Updated Proposed fix to include unregister (Pawel's solution) (15.30 KB, patch)
2011-04-04 10:08 EDT, Marc Khouzam CLA
marc.khouzam: iplog-
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Khouzam CLA 2011-04-01 14:11:59 EDT
This bug is a spin-off of bug 341423 because we need to wait for the next release to make API changes.

The summary of the problem is it can happen that a service is used before it is fully initialized.

Two solutions were proposed in bug 341423.  The current thought is that we would go with Pawel's solution and make AbstractDsfService.register() private.
Comment 1 Marc Khouzam CLA 2011-04-01 15:51:58 EDT
Created attachment 192385 [details]
Proposed fix (Pawel's solution)

Attached is a cleaned-up proposed fix, based on Pawel's solution. It modifies AbstractDsfService and it gives an example of the use of the changes in DSF-GDB.  This is still a proposal that we'll have to agree or disagree on when we start working on the next release.

P.S. I'm not attached to any of the names I chose, so please feel free to suggest other names if you wish.

Things to note:
==============
-API breakage
  - As mentioned, AbstractDsfService.initialize() goes from "public" to "public final".
  - AbstractDsfService.register() goes from "protected" to "private"
  - new abstract method AbstractDsfService.getClassNamesToRegiter()

- AbstractDsfService.register() now takes a list of names instead of an array to simplify things.
- AbstractDsfService.register() no longer supports being called more than once.

Things to decide:
================
1- Should register() modify the list returned by getClassNamesToRegiter() or should it make a copy first?  Note that register() modifies the list of properties. 

2- I don't think register() should automatically add the actual class type of this object as a registered name anymore.  We should leave this up to the service itself.  The reason is that with the new getClassNamesToRegiter() method, I can see someone doing something like this:
        
         BaseClass {
           protected List<String> getClassNamesToRegiter() {
              return new ArrayList<String>();
              // Here the designer did not explicitly add
              // BaseClass.class.getName() to the list of names to register
              // because he relied on AbstractDsfService.register() to add
              // it automatically.
           }
         };
        
         DerivedClass extends baseClass {
           protected List<String> getClassNamesToRegiter() {
              List<String> classes = super.getClassNamesToRegiter();
              
              // PROBLEM, classes does not contain BaseClass.class.getName()
              // and AbstractDsfService.register() won't add it!
              
              classes.add(DerivedClass.class.getName());
              return classes;
           }
         };
Comment 2 Marc Khouzam CLA 2011-04-01 16:03:56 EDT
Another thing to note is that based on a suggestion from Bug 341465, I made the new AbstractDsfService.getClassNamesToRegiter() return a list instead of an array.  This makes it easier for extenders which can simply add a new name to register instead of writing out the full list each time.
Comment 3 Marc Khouzam CLA 2011-04-01 21:01:57 EDT
I just realized that we also need to make some change for unregister().  I think it should be private and automatically be called from shutdown()
Comment 4 Anton Leherbauer CLA 2011-04-04 02:54:17 EDT
> getClassNamesToRegiter

Typo?
Comment 5 Marc Khouzam CLA 2011-04-04 10:08:50 EDT
Created attachment 192458 [details]
Updated Proposed fix to include unregister (Pawel's solution)

(In reply to comment #4)
> > getClassNamesToRegiter
> 
> Typo?

Thanks!  getClassNamesToRegiter() was wrong, and also getPropertiesToRegiter().

Here is the updated patch that fixes the two typos and two other things:

1- I applied the same new pattern to shutdown().  I made shutdown() final, unregister() private, and I created performShutdown().  This allows to automatically unregister, just like we would automatically register().  It just does not make sense to ask an extender to perform the unregister() if it no longer explicitly does the register().

2- I added a check that getClassNamesToRegister() != null before trying to automatically register.  This would allow someone to prevent registering with OSGi if that ever is needed.  Note that unregister() has a check to see if we were previously registered, so we can do that one blindly.

This is all still a proposal that we should agree on when we start working on Juno (next release).