NEW DATE! Bugzilla will undergo maintenance 2024-03-28 18h00 CET. Bugzilla will be placed in read-only mode at that time.

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 485107 - Extend GdbAdapterFactory in a way that does not cause multiple factories to load
Summary: Extend GdbAdapterFactory in a way that does not cause multiple factories to load
Status: NEW
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: 9.0.0   Edit
Hardware: PC Windows NT
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact: Jonah Graham CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-04 02:04 EST by Jonah Graham CLA
Modified: 2020-09-04 15:18 EDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonah Graham CLA 2016-01-04 02:04:20 EST
Reproduced from Marc's comment in gerrit: https://git.eclipse.org/r/#/c/61534/8/dsf-gdb/org.eclipse.cdt.examples.dsf.gdb/plugin.xml

If you dig a little deeper, you will probably wonder why DsfTerminateCommand (and even more, DsfExtendedTerminateCommand) even gets called when the GdbExtendedLaunch node is selected, when that adapter type is not even listed above.  
Bottom line is that the extending of the GdbAdapterFactory is not done well.  I hadn't realized but it turns out that both GdbAdapterFactory and GdbExtendedAdapterFactory get instantiated.  Things work for two reasons:
1- the extended factory is called first and gets to create all the adapters; so DsfExtendedTerminateCommand gets created properly.  This may be a simply lucky, I'm not sure. If GdbAdapterFactory were to get called first, the extended adapters wouldn't be used.
2- When GdbExtendedLaunch is selected in the debug view, GdbExtendedAdapterFactory will not trigger to enable the Terminate command BUT since GdbExtendedLaunch is also a GdbLaunch, GdbAdapterFactory does get triggered, and as it happens to share a static list of adapters, it finds DsfExtendedTerminateCommand!
It would be nice to improve this, to provide a clean way to extend our list of adapters.
Comment 1 Jonah Graham CLA 2016-01-04 04:02:45 EST
With further reference to https://git.eclipse.org/r/#/c/61534, it may be worth adding back in adapter for launch to IShowVersionHandler to demonstrate functioning of extending adapter factory.
Comment 2 Marc Khouzam CLA 2016-03-31 14:27:00 EDT
(In reply to Jonah Graham from comment #0)

> I hadn't realized but it turns out that both GdbAdapterFactory and
> GdbExtendedAdapterFactory get instantiated.  Things work for two reasons:
> 1- the extended factory is called first and gets to create all the adapters;
> so DsfExtendedTerminateCommand gets created properly.  This may be a simply
> lucky, I'm not sure. If GdbAdapterFactory were to get called first, the
> extended adapters wouldn't be used.

It turns out that the first call to GdbAdapterFactory.getAdapter() is for org.eclipse.debug.ui.contexts.ISuspendTrigger, and for some reason, the platform chooses GdbExtendedAdapterFactory to call first.  So, things work as explained above.

However, if I remove ISuspendTrigger from the list of adapterTypes handled by GdbExtendedAdapterFactory from its plugin.xml, then it is GdbAdapterFactory that gets called first and creates all adapters.  This means that DsfExtendedTerminateCommand never gets created, and pressing Terminate on a GdbExtendedLaunch no longer pops up the confirmation dialogue as implemented by DsfExtendedTerminateCommand.

This looks quite brittle to me.

I'm looking into a fix.
Comment 3 Marc Khouzam CLA 2016-03-31 15:39:10 EDT
(In reply to Jonah Graham from comment #0)

> Bottom line is that the extending of the GdbAdapterFactory is not done well.
> I hadn't realized but it turns out that both GdbAdapterFactory and
> GdbExtendedAdapterFactory get instantiated.

The fundamental problem is that we register GdbAdapterFactory with GdbLaunch in the plugin.xml file.  This means that any launch extending GdbLaunch will get that factory, even if they want to provide their own.

The immediate thought is that extenders should not override GdbLaunch.  But that is not good for multiple reasons:

1- There are many places in the code where we do "instancedof GdbLaunch" and we want those to continue working with a launch from an extender.

2- Extenders should not have to duplicate all the code from GdbLaunch in their own launch.  They really should be able to extend it.

So, the next thought is that GdbAdapterFactory should not be associated with GdbLaunch, but with another launch that extenders will not subclass.  The idea would be


                       GdbLaunch - No automatic factory anymore
                        /      \
 GdbLaunchWithAdapterFactory   ExtendersLaunch that want their own factory
     associated with    \
    GdbAdapterFactory    \ 
        automatically    ExtendersLaunch that want to re-use GdbAdapterFactory


I've written a patch for this and it works well.
However, it has drawbacks:

1- Any existing ExtendersLaunch that don't provide their own adapterFactory, will stop working, as they don't currently extend GdbLaunchWithAdapterFactory.  They would need to consciously make this change for things to work.  The change is very minor and they will notice right away things are broken.  Still, this is probably not very nice.

2- Extenders that don't provide their own factory and want to support CDT 9.0 AND older CDT's will need to do something special.  They will not be able to extend a new GdbLaunchWithAdapterFactory as it doesn't exist in older CDTs.  But they still need to do something, or else they will not get the GdbAdapterFactory automatically when using CDT 9.0.  So they will need to add the GdbAdapterFactory association in their own plugin.xml.

3- If they ever want to start providing their own factory after the fact, they will need to remember to change their inheritance from GdbLaunchWithAdapterFactory to GdbLaunch, which I don't expect anyone to remember.  However, if they don't, things won't be worse than what they are today, so maybe that is not a blocker.

I think 2 and 3 we can live with, but I think #1 is a big deal.  

Maybe a different approach should be taken.  I'll keep thinking.
Comment 4 Eclipse Genie CLA 2016-03-31 16:05:39 EDT
New Gerrit change created: https://git.eclipse.org/r/69658
Comment 5 Marc Khouzam CLA 2016-03-31 23:47:27 EDT
(In reply to Marc Khouzam from comment #3)

> Maybe a different approach should be taken.  I'll keep thinking.

I came up with something else.

The idea is to work around the problem, since the cleaner fix has too great an impact on existing extended launches.

Although the root of the problem is that we register GdbAdapterFactory with GdbLaunch in the plugin.xml file.  The practical impact is that in some cases, GdbAdapterFactory will be used to create all the adapters instead of using the extended version of that class as provided by extenders.

So, I worked on a fix that allows to re-create the set of adapters if a second factory is found and if that factory is a sub-class of GdbAdapterFactory.

There are two scenarios:
1- The extended factory is called first and gets to create the adapters.  This case already works, as it is the extended factory that has precedence.

2- The base GdbAdapterFactory is called first and creates the adapters.  This is the broken case where extenders are unable to replace the adapters with their own.  With the new solution, when the extended factory is called on a second turn, instead of being ignored because the adapter exist already, the code will check if that factory is extending the currently selected one; if it does, then it will assume it has precedence, will delete the created adapters, and will re-create them using the extended factory.

I've pushed this proposal to gerrit in:
 https://git.eclipse.org/r/69660

One thing to realize is that GdbAdaterFactory and the extended factory will both be active still.  However, the right set of adapters will have been created.  Furthermore, after some testing, I confirmed that the extended factory is the one that will be called when selecting a factory to adapt the extended GdbLaunch. So even though both factories are registered with an extended GdbLaunch (because GdbAdapterFactory is registered with GdbLaunch and that the extendedGdbLaunch is also a GdbLaunch), it is the extended factory that gets called when wanting to adapt extendedGdbLaunch.

So, both uses of extendedGdbAdapterFactory work when using this proposed fix.

Jonah, when you have some time, could you have a look?
Comment 6 Jonah Graham CLA 2016-04-05 08:36:29 EDT
(In reply to Marc Khouzam from comment #5)
> Jonah, when you have some time, could you have a look?
Hi Marc, I am looking at this now.
Comment 7 Jonah Graham CLA 2016-04-05 10:20:43 EDT
I think we have over complicated this a bit.

AFAICT the problem here is that the GdbExtendedLaunch does not list org.eclipse.debug.core.commands.ITerminateHandler (and others) in the list of classes GdbExtendedLaunch can adapt to (in /org.eclipse.cdt.examples.dsf.gdb/plugin.xml)

The AdapterManager decides which factory to use based on a defined order, see javadoc on AdapterManager [1]. In the case of GdbExtendedLaunch with the current plugin.xml the table [2] looks like this:

Adapted Type --> Factory to use
ITerminateHandler --> GdbAdapterFactory
ISuspendTrigger --> GdbExtendedAdapterFactory

This is the bug, the ITerminateHandler should not be resolved to GdbAdapterFactory at all.

If you simply provide the missing types to the plugin.xml of the GdbExtendedLaunch then the table looks like this:

Adapted Type --> Factory to use
ITerminateHandler --> GdbExtendedAdapterFactory
ISuspendTrigger --> GdbExtendedAdapterFactory

And since the GdbExtendedAdapterFactory is never referenced, there is no problem of multiple loading factories, and therefore reloading adapter sets is not needed.

I will submit a gerrit that fixes this issue in the first instance.

Note that referring back to Eugene Ostroukhov's recommendation on how to extend the factory implies that all the adpater types need to be listed [3]


[1] https://git.eclipse.org/c/equinox/rt.equinox.bundles.git/tree/bundles/org.eclipse.equinox.common/src/org/eclipse/core/internal/runtime/AdapterManager.java#n20

[2] The tables is stored once constructed in org.eclipse.core.internal.runtime.AdapterManager.adapterLookup with the key being the adaptable type, i.e. org.eclipse.cdt.examples.dsf.gdb.launch.GdbExtendedLaunch in this case

[3] https://dev.eclipse.org/mhonarc/lists/cdt-dev/msg23664.html
Comment 8 Eclipse Genie CLA 2016-04-05 10:27:42 EDT
New Gerrit change created: https://git.eclipse.org/r/69923
Comment 9 Jonah Graham CLA 2016-04-05 10:35:25 EDT
Assuming that my last gerrit https://git.eclipse.org/r/69923 is on the right track, I have an idea of how to handle this programatically so that extenders don't have to duplicate the information. I will create another gerrit for review, but probably not before the CDT call this afternoon at which this may be discussed.
Comment 10 Marc Khouzam CLA 2016-04-05 11:03:51 EDT
(In reply to Jonah Graham from comment #7)
> 
> The AdapterManager decides which factory to use based on a defined order,
> see javadoc on AdapterManager [1]. 

Thanks for digging into this.  I assumed the order was unspecified, but I'm glad it is not.

> In the case of GdbExtendedLaunch with the
> current plugin.xml the table [2] looks like this:
> 
> Adapted Type --> Factory to use
> ITerminateHandler --> GdbAdapterFactory
> ISuspendTrigger --> GdbExtendedAdapterFactory
> 
> This is the bug, the ITerminateHandler should not be resolved to
> GdbAdapterFactory at all.

Well, this is the bug you hit :)
The bugzilla attempts to fix a larger problem if both factories being instantiated, which normally is not a problem, but in our case could be.

> And since the GdbExtendedAdapterFactory is never referenced, there is no
> problem of multiple loading factories, and therefore reloading adapter sets
> is not needed.

I assume that above you mean "since the GdbAdapterFactory is never referenced".
I agree with that statement for the GdbExtendedAdapterFactory case.

Maybe I'm being overly cautious, but I'm worried about the same case for factories that we don't control, the ones implemented by extenders.  Say there is one such factory called ExternalAdapterFactory which extends GdbAdapterFactory.

Say CDT adds a new adapter that applies to the launch, like we did when adding IDebugNewExecutableHandler.  Then GdbAdapterFactory would be modified to support that new adapter.  ExternalAdapterFactory would not be modified automatically, since, in this example, we don't know it even exists.

The table then becomes

Adapted Type --> Factory to use
IDebugNewExecutableHandler  --> GdbAdapterFactory
ITerminateHandler           --> ExternalAdapterFactory
ISuspendTrigger             --> ExternalAdapterFactory

If IAdapterFactory.getAdapter(Object, IDebugNewExecutableHandler) happens to be called first, then it will get GdbAdapterFactory.createGdbSessionAdapters() and instantiate the wrong adapter for all the session, loosing any overridden adapters from ExternalAdapterFactory.

This is why I was thinking we needed to provide extenders with a way to make sure GdbAdapterFactory was not loaded instead of their own.

What do you think?
Comment 11 Jonah Graham CLA 2016-04-05 11:17:08 EDT
(In reply to Marc Khouzam from comment #10)
> This is why I was thinking we needed to provide extenders with a way to make
> sure GdbAdapterFactory was not loaded instead of their own.
> 
> What do you think?
 
If we programatically register the adapter factory with the manager then the manager uses IAdapterFactory.getAdapterList() to get the list of types that can be adapted to. If the factory is only registered with the manager declaratively then IAdapterFactory.getAdapterList() is not called.

By doing it programatically we can add new types to the list and all extenders will pick it up with no changes.

I will do a gerrit for this soon.
Comment 12 Eclipse Genie CLA 2016-04-27 12:14:26 EDT
New Gerrit change created: https://git.eclipse.org/r/71533