Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom ITransportProvider cannot be found by ServiceLoader #251

Open
brett-smith opened this issue Mar 5, 2024 · 4 comments
Open

Custom ITransportProvider cannot be found by ServiceLoader #251

brett-smith opened this issue Mar 5, 2024 · 4 comments

Comments

@brett-smith
Copy link
Contributor

brett-smith commented Mar 5, 2024

We have a problem when trying to use a custom transport provider inside an application that makes use of ModuleLayer. The exception is ..

No transport provider found for bustype SSH
    at org.freedesktop.dbus/org.freedesktop.dbus.connections.transports.TransportBuilder.build(TransportBuilder.java:177)
    at org.freedesktop.dbus/org.freedesktop.dbus.connections.base.AbstractConnectionBase.<init>(AbstractConnectionBase.java:114)
    at org.freedesktop.dbus/org.freedesktop.dbus.connections.base.ConnectionMethodInvocation.<init>(ConnectionMethodInvocation.java:33)
    at org.freedesktop.dbus/org.freedesktop.dbus.connections.base.ConnectionMessageHandler.<init>(ConnectionMessageHandler.java:42)
    at org.freedesktop.dbus/org.freedesktop.dbus.connections.AbstractConnection.<init>(AbstractConnection.java:41)

The issue is in TransportGuilder.getTransportProvider(). This uses ServiceLoader.load(ITransportProvider.class, TransportBuilder.class.getClassLoader()) to locate the services.

This will only find ITransportProvider implementations that exist in the same layer, or in a parent layer that is associated with TransportBuilder.class.getClassLoader(). My provider is in a child layer, so will never be found.

So there is a need to be able to use the alternative methods provided by ServiceLoader that take a ModuleLayer as the first argument. Also, I would imagine it would be useful to pass alternative ClassLoader instead for applications that don't make use of module layers, but do have different class loader arrangements (e..g pf4j based applications or other plugin frameworks).

That static nature of TransportBuilder.getTransportProvider() and when it is called looks like this might be a moderately challenging fix, so I am not going to dive in and make a PR without discussion.

Initial thoughts are that it looks like this configuration could possibly be passed down in TransportConfig, but there would need to be some rearranging of how the constant PROVIDERS is dealt with.

@hypfvieh
Copy link
Owner

hypfvieh commented Mar 5, 2024

Sounds like a good improvement.
The reason ServiceLoader is used with TransportBuilder.class.getClassLoader() was that there were issues with the OSGi crap some years ago.

It would be great to support user defined classloaders or the module system properly.
If you have any idea on how to get rid of the static member and methods without breaking anything please go ahead.

I tried some ideas but I'm not really happy with it.
How do I know if a implementation has to be looked up by a classloader or module layer when only having a BusAddress or String object?
One option would be deprecating these methods or assume to do it like it worked before (using TransportBuilder.class.getClassLoader()).
Also using static methods or members will probably conflict with the new configuration options for ServiceLoader.

I pushed my code to "serviceloader-update" branch. Maybe you take a look and improve it if you have good ideas.

@brett-smith
Copy link
Contributor Author

How do I know if a implementation has to be looked up by a classloader or module layer when only having a BusAddress or String object?

I don't think you can. It's up to the caller to provide the right layer or classloader in addition to the address.

This doesn't matter in my particular use-case at least. We already know we are in a layered application, and have to do some special things (nearly always class loader related!).

Over the years, other third party libraries we have dealt with have similar issues locating services and resources. Many of them allow class loader to be specified directly, a few allow a ModuleLayer , but only because its "new". Quite often Thread.setContextClassLoader(loader) is supported. Perhaps that particular pattern or something similar could be used here., but personally I would prefer something better.

I pushed my code to "serviceloader-update" branch. Maybe you take a look and improve it if you have good ideas.

Will do!

@brett-smith
Copy link
Contributor Author

Perfect, that's behaving exactly as I'd expect. And the configuration is easily discoverable.

I would not bother with any further changes. Anyone working with classloaders is going to expect stuff like this, and the fact it is now possible to do at all is great.

I consider this fixed, and look forward to it making it into a snapshot or release. Thanks for the super speed response too.

@hypfvieh
Copy link
Owner

hypfvieh commented Mar 6, 2024

Merged branch to master, will be part of next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants