Attachment 192458 Details for Bug 341660 – Updated Proposed fix to include unregister (Pawel's solution)

[patch] Updated Proposed fix to include unregister (Pawel's solution)

z.341660.patch (text/plain), 15.30 KB, created by Marc Khouzam

on 2011-04-04 10:08:50 EDT

(hide)

Description:

Filename:

MIME Type:

Creator: Marc Khouzam

Created: 2011-04-04 10:08:50 EDT

Size: 15.30 KB

patch

obsolete

>### Eclipse Workspace Patch 1.0
>#P org.eclipse.cdt.dsf
>Index: src/org/eclipse/cdt/dsf/service/AbstractDsfService.java
>===================================================================
>RCS file: /cvsroot/tools/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/service/AbstractDsfService.java,v
>retrieving revision 1.11
>diff -u -r1.11 AbstractDsfService.java
>--- src/org/eclipse/cdt/dsf/service/AbstractDsfService.java	3 Jun 2010 00:11:48 -0000	1.11
>+++ src/org/eclipse/cdt/dsf/service/AbstractDsfService.java	4 Apr 2011 14:02:21 -0000
>@@ -10,14 +10,14 @@
>  *******************************************************************************/
> package org.eclipse.cdt.dsf.service;
> 
>-import java.util.Arrays;
> import java.util.Dictionary;
> import java.util.Enumeration;
>-import java.util.HashSet;
>-import java.util.Set;
>+import java.util.Hashtable;
>+import java.util.List;
> 
> import org.eclipse.cdt.dsf.concurrent.DsfExecutor;
> import org.eclipse.cdt.dsf.concurrent.IDsfStatusConstants;
>+import org.eclipse.cdt.dsf.concurrent.ImmediateExecutor;
> import org.eclipse.cdt.dsf.concurrent.RequestMonitor;
> import org.osgi.framework.BundleContext;
> import org.osgi.framework.Constants;
>@@ -65,7 +65,7 @@
>     public DsfExecutor getExecutor() { return fSession.getExecutor(); }
> 
> 	/**
>-	 * The the returned collection is a superset of the properties specified in
>+	 * The returned collection is a superset of the properties specified in
> 	 * {@link #register(String[], Dictionary)}. That method can add additional
> 	 * (implicit) properties. For one, it tacks on the
> 	 * {@link Constants#OBJECTCLASS} property associated with the service after
>@@ -81,18 +81,70 @@
>     
>     public int getStartupNumber() { return fStartupNumber; }
>     
>-    public void initialize(RequestMonitor rm) {
>+    /** 
>+     * @return The class names under which the service can be located. For
>+	 *         convenience, [classes] need not contain {@link IDsfService},
>+	 *         it is automatically added if not present. The complete list 
>+	 *         of classes the service ends up being registered in OSGi with
>+	 *         is recorded and made available via
>+	 *         <code>getProperties().get(Constants.OBJECTCLASS)</code>
>+     */
>+    abstract protected List<String> getClassNamesToRegister();
>+
>+    /**
>+     * Sub-classes can override this method to specify properties to use for
>+     * the registration of the service, or to obtain directly the final
>+     * list of properties that was used to register the service.
>+     * 
>+     * @return The properties for this service. The keys in the properties
>+	 *         object must all be <code>String</code> objects. See
>+	 *         {@link Constants} for a list of standard service property
>+	 *         keys. To update the service's properties after initialization
>+	 *         the {@link ServiceRegistration#setProperties} method must be
>+	 *         called. Caller should, at a minimum, pass an empty
>+	 *         dictionary--never null. We add a property to the collection
>+	 *         (we modify the list returned by this method), to record the id of
>+	 *         the dsf session associated with the service as well as all the names
>+	 *         of the classes the service ends up being registered in OSGi with.
>+	 */
>+    @SuppressWarnings("rawtypes")
>+    protected Dictionary getPropertiesToRegister() {
>+    	return new Hashtable<String, String>();
>+    }
>+
>+    final public void initialize(final RequestMonitor rm) {
>         fTracker = new DsfServicesTracker(getBundleContext(), fSession.getId());
>         fStartupNumber = fSession.getAndIncrementServiceStartupCounter();
>+        performInitialization(
>+                new RequestMonitor(ImmediateExecutor.getInstance(), rm) {
>+                    @Override
>+                    protected void handleSuccess() {
>+                    	if (getClassNamesToRegister() != null) {
>+                    		// Once all the initialization has been performed, we need to register
>+                    		// the service to make it available to be used.
>+                    		register(getClassNamesToRegister(), getPropertiesToRegister());
>+                    	}
>+                        rm.done();
>+                    }});
>+    }
>+    
>+    /** @since 2.2 */
>+    protected void performInitialization(RequestMonitor rm) {
>         rm.done();
>     }
>         
>-    public void shutdown(RequestMonitor rm) {
>-        fTracker.dispose();
>-        fTracker = null;
>+    final public void shutdown(RequestMonitor rm) {
>+    	unregister();
>+    	performShutdown(rm);
>+    }
>+
>+    /** @since 2.2 */
>+    protected void performShutdown(RequestMonitor rm) {
>+    	fTracker.dispose();
>+    	fTracker = null;
>         rm.done();
>     }
>-    
>+
>     /**
> 	 * @since 2.0
> 	 */
>@@ -117,10 +169,11 @@
> 	 * 
> 	 * @param classes
> 	 *            The class names under which the service can be located. For
>-	 *            convenience, [classes] need not contain {@link IDsfService} or
>-	 *            the runtime class of this object; they are automatically added
>-	 *            if not present. The complete list of classes the service ends
>-	 *            up being registered in OSGi with is recorded and made
>+	 *            convenience, [classes] need not contain {@link IDsfService} 
>+	 *            TODO or the runtime class of this object; they are automatically added
>+	 *            if not present (we modify the caller's object). 
>+	 *            The complete list of classes the service ends
>+	 *            up being registered in OSGi with is recorded and also made
> 	 *            available via
> 	 *            <code>getProperties().get(Constants.OBJECTCLASS)</code>
> 	 * @param properties
>@@ -136,52 +189,50 @@
> 	 *            session associated with the service.
> 	 */
>     @SuppressWarnings({ "rawtypes", "unchecked" })
>-    protected void register(String[] classes, Dictionary properties) {
>+    private void register(List<String> classes, Dictionary properties) {
>+    	assert fRegistration == null;
>     	
>-    	/*
>-    	 * If this service has already been registered, make sure we
>-    	 * keep the names it has been registered with.  However, we
>-    	 * must trigger a new registration or else OSGI will keep the two
>-    	 * registration separate. 
>-    	 */
>-    	if (fRegistration != null) {
>-    		String[] previousClasses = (String[])fRegistration.getReference().getProperty(Constants.OBJECTCLASS);
>-    		
>-            // Use a HashSet to avoid duplicates
>-        	Set<String> newClasses = new HashSet<String>();
>-        	newClasses.addAll(Arrays.asList(previousClasses));
>-        	newClasses.addAll(Arrays.asList(classes));
>-        	classes = newClasses.toArray(new String[0]);
>-
>-        	/*
>-        	 * Also keep all previous properties.
>-        	 */
>-            if (fProperties != null) {
>-            	for (Enumeration e = fProperties.keys() ; e.hasMoreElements();) {
>-            		Object key = e.nextElement();
>-            		Object value = fProperties.get(key);
>-            		properties.put(key, value);
>-            	}
>-            }
>-            
>-        	// Now, cancel the previous registration
>-    		unregister();
>-    	}
>         /*
>-         * Ensure that the list of classes contains the base DSF service 
>-         * interface, as well as the actual class type of this object.
>+         * Ensure that the list of classes contains the base DSF service interface.
>          */
>-        if (!Arrays.asList(classes).contains(IDsfService.class.getName())) {
>-            String[] newClasses = new String[classes.length + 1];
>-            System.arraycopy(classes, 0, newClasses, 1, classes.length);
>-            newClasses[0] = IDsfService.class.getName();
>-            classes = newClasses;
>+        if (!classes.contains(IDsfService.class.getName())) {
>+            classes.add(0, IDsfService.class.getName());
>         }
>-        if (!Arrays.asList(classes).contains(getClass().getName())) {
>-            String[] newClasses = new String[classes.length + 1];
>-            System.arraycopy(classes, 0, newClasses, 1, classes.length);
>-            newClasses[0] = getClass().getName();
>-            classes = newClasses;
>+
>+        /*
>+          TODO
>+          I think we should remove this, although it may cause problems to existing extenders.
>+         
>+         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;
>+           }
>+         };
>+         */
>+        /*
>+         * Ensure that the list of classes contains the actual class type of this object.
>+         */        
>+        if (!classes.contains(getClass().getName())) {
>+        	classes.add(0, getClass().getName());
>         }
>         /*
>          * Make sure that the session ID is set in the service properties.
>@@ -190,7 +241,7 @@
>          */
>         properties.put(PROP_SESSION_ID, getSession().getId());
>         fProperties = properties;
>-        fRegistration = getBundleContext().registerService(classes, this, properties);
>+        fRegistration = getBundleContext().registerService(classes.toArray(new String[classes.size()]), this, properties);
>         
>         /*
>          * Retrieve the OBJECTCLASS property directly from the service 
>@@ -248,7 +299,7 @@
>      * De-registers this service.
>      *
>      */
>-    protected void unregister() {
>+    private void unregister() {
>     	if (fRegistration != null) {
>     		fRegistration.unregister();
>     	}
>#P org.eclipse.cdt.dsf.gdb
>Index: src/org/eclipse/cdt/dsf/gdb/service/GDBBreakpoints_7_0.java
>===================================================================
>RCS file: /cvsroot/tools/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBreakpoints_7_0.java,v
>retrieving revision 1.10
>diff -u -r1.10 GDBBreakpoints_7_0.java
>--- src/org/eclipse/cdt/dsf/gdb/service/GDBBreakpoints_7_0.java	1 Apr 2011 18:17:41 -0000	1.10
>+++ src/org/eclipse/cdt/dsf/gdb/service/GDBBreakpoints_7_0.java	4 Apr 2011 14:02:22 -0000
>@@ -12,7 +12,7 @@
> 
> import java.util.ArrayList;
> import java.util.HashMap;
>-import java.util.Hashtable;
>+import java.util.List;
> import java.util.Map;
> 
> import org.eclipse.cdt.dsf.concurrent.CountingRequestMonitor;
>@@ -22,8 +22,6 @@
> import org.eclipse.cdt.dsf.concurrent.Sequence.Step;
> import org.eclipse.cdt.dsf.datamodel.DMContexts;
> import org.eclipse.cdt.dsf.datamodel.IDMContext;
>-import org.eclipse.cdt.dsf.debug.service.IBreakpoints;
>-import org.eclipse.cdt.dsf.debug.service.IBreakpointsExtension;
> import org.eclipse.cdt.dsf.debug.service.command.ICommandControl;
> import org.eclipse.cdt.dsf.gdb.internal.GdbPlugin;
> import org.eclipse.cdt.dsf.gdb.internal.tracepointactions.ITracepointAction;
>@@ -59,11 +57,12 @@
> 	}
> 
> 	/* (non-Javadoc)
>-	 * @see org.eclipse.cdt.dsf.service.AbstractDsfService#initialize(org.eclipse.cdt.dsf.concurrent.RequestMonitor)
>+	 * @see org.eclipse.cdt.dsf.service.AbstractDsfService#performInitialization
>+	 * (org.eclipse.cdt.dsf.concurrent.RequestMonitor)
> 	 */
> 	@Override
>-	public void initialize(final RequestMonitor rm) {
>-		super.initialize(new RequestMonitor(ImmediateExecutor.getInstance(), rm) {
>+	protected void performInitialization(final RequestMonitor rm) {
>+		super.performInitialization(new RequestMonitor(ImmediateExecutor.getInstance(), rm) {			
> 			@Override
> 			protected void handleSuccess() {
> 				doInitialize(rm);
>@@ -77,20 +76,19 @@
> 		fRunControl = getServicesTracker().getService(IMIRunControl.class);
>         fCommandFactory = getServicesTracker().getService(IMICommandControl.class).getCommandFactory();
> 
>-		// Register this service
>-		register(new String[] { IBreakpoints.class.getName(),
>-		                        IBreakpointsExtension.class.getName(),
>-								MIBreakpoints.class.getName(),
>-				 				GDBBreakpoints_7_0.class.getName() },
>-				new Hashtable<String, String>());
>-
> 		rm.done();
> 	}
>+	
>+	@Override
>+	protected List<String> getClassNamesToRegister() {
>+		List<String> classes = super.getClassNamesToRegister();
>+		classes.add(GDBBreakpoints_7_0.class.getName());
>+		return classes;
>+	}
> 
> 	@Override
>-	public void shutdown(RequestMonitor requestMonitor) {
>-        unregister();
>-		super.shutdown(requestMonitor);
>+	public void performShutdown(RequestMonitor requestMonitor) {
>+		super.performShutdown(requestMonitor);
> 	}
> 
> 	/**
>Index: src/org/eclipse/cdt/dsf/mi/service/MIBreakpoints.java
>===================================================================
>RCS file: /cvsroot/tools/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/MIBreakpoints.java,v
>retrieving revision 1.18
>diff -u -r1.18 MIBreakpoints.java
>--- src/org/eclipse/cdt/dsf/mi/service/MIBreakpoints.java	1 Apr 2011 18:17:41 -0000	1.18
>+++ src/org/eclipse/cdt/dsf/mi/service/MIBreakpoints.java	4 Apr 2011 14:02:22 -0000
>@@ -13,7 +13,6 @@
> 
> import java.util.ArrayList;
> import java.util.HashMap;
>-import java.util.Hashtable;
> import java.util.Iterator;
> import java.util.List;
> import java.util.Map;
>@@ -289,11 +288,11 @@
> 	}
> 
> 	/* (non-Javadoc)
>-	 * @see org.eclipse.cdt.dsf.service.AbstractDsfService#initialize(org.eclipse.cdt.dsf.concurrent.RequestMonitor)
>+	 * @see org.eclipse.cdt.dsf.service.AbstractDsfService#performInitialization(org.eclipse.cdt.dsf.concurrent.RequestMonitor)
> 	 */
> 	@Override
>-	public void initialize(final RequestMonitor rm) {
>-		super.initialize(new RequestMonitor(ImmediateExecutor.getInstance(), rm) {
>+	protected void performInitialization(final RequestMonitor rm) {
>+		super.performInitialization(new RequestMonitor(ImmediateExecutor.getInstance(), rm) {			
> 			@Override
> 			protected void handleSuccess() {
> 				doInitialize(rm);
>@@ -315,21 +314,26 @@
>         // Register for the useful events
> 		getSession().addServiceEventListener(this, null);
> 
>-		// Register this service
>-		register(new String[] { IBreakpoints.class.getName(), IBreakpointsExtension.class.getName(), MIBreakpoints.class.getName() },
>-				new Hashtable<String, String>());
>-
> 		rm.done();
> 	}
> 
>+	@Override
>+	protected List<String> getClassNamesToRegister() {
>+		List<String> names = new ArrayList<String>();
>+		names.add(IBreakpoints.class.getName());
>+		names.add(IBreakpointsExtension.class.getName());
>+		names.add(MIBreakpoints.class.getName());
>+
>+		return names;
>+	}
>+	
> 	/* (non-Javadoc)
>-	 * @see org.eclipse.cdt.dsf.service.AbstractDsfService#shutdown(org.eclipse.cdt.dsf.concurrent.RequestMonitor)
>+	 * @see org.eclipse.cdt.dsf.service.AbstractDsfService#performShutdown(org.eclipse.cdt.dsf.concurrent.RequestMonitor)
> 	 */
> 	@Override
>-	public void shutdown(final RequestMonitor rm) {
>-		unregister();
>+	protected void performShutdown(final RequestMonitor rm) {
> 		getSession().removeServiceEventListener(this);
>-		super.shutdown(rm);
>+		super.performShutdown(rm);
> 	}
> 
> 	/* (non-Javadoc)

Flags:

marc.khouzam: iplog-

Actions: View | Diff

Attachments on bug 341660: 192385 | 192458