Provide an API to ContainerProvider to allow ServiceLoader results to be filtered / prioritised / sorted

[markt-asf]

This will require a minimum of Java 9. Then we can use ServiceLoader.stream() and filter / sort the results. I'm leaning towards sort rather than filter.

[joakime]

How would that be used?
Got any pseudo code on how a user of the websocket-api could use such a thing?

Also, last time I looked into it, ServiceLoader.stream() will still attempt to instantiate the class when you iterate over it.

[markt-asf]

stream() loads the class but does not instantiate an instance of the provider.
Usage something like:

WebSocketContainer result = null;
        
ServiceLoader<ContainerProvider> loader = ServiceLoader.load(ContainerProvider.class);
Stream<Provider<ContainerProvider>> stream = loader.stream();
Iterator<Provider<ContainerProvider>> providers = stream.sorted(comparator).iterator();
while (result == null && providers.hasNext()) {
        result = providers.next().get().getContainer();
}
        
return result;

with static getter/setter for comparator

I'm open to alternative solutions that allow the user to control which implementation is used when multiple implementations are available.

[jansupol]

+1 for priorities, -1 for using stream().
JDK 8 is supposed to be supported until 2030 and I do not believe Jakarta EE 10 will set the minimum JDK version to 11. I do not see a reason for using stream(), ServiceLoader#iterator() seems to be adequate for the task.

[joakime]

Wouldn't this be easier with a ContainerProvider.setImplementation(Class<? extends WebSocketContainer> implementation)?

@jansupol The Jakarta EE 10 minimum JDK has been brought up a few times in the various mailing lists.
It's been pointed out that there are already specs making changes that do not work on JDK 8.
Also, every container out there supports JDK 11 now.

I believe that those groups wanting to stick with JDK 8 will have to stay with older Jakarta EE revisions.

[joakime]

For the record, I'm -1 for priorities in the websocket API itself, and would rather see that be something a user of the API implements on top of the API if they desire it.

Alternative 2 - how about using a Predicate instead?

Either as a ContainerProvider singleton behavior change for all users of ContainerProvider.getWebSocketContainer(), even other libraries within your Classloader.
I would love to see this approach, but I don't think it would work really all that well as ContainerProvider is an interface.

void ContainerProvider.setImplementationPredicate(Predicate<Class<? extends WebSocketContainer>> predicate)

Or as a per-call version (this could be baked into the spec provided ContainerProvider interface) ...

WebSocketContainer ContainerProvider.getWebSocketContainer(Predicate<Class<? extends WebSocketContainer>> predicate)

The per-call version could benefit from having a default predicate that just returns the first hit (like it currently does).

[markt-asf]

I think it would be useful to get @rmannibucau 's view on this since the requirement originated with TomEE.
My impression is that Predicate seems unnecessarily restrictive. It may work for the original use case but what is it that makes Predicate acceptable but Comparator not?

[joakime]

I agree, more details would be useful to frame the discussion further.

From the limited information so far, it seems that for WebSocket Client usage there are situations where there are multiple implementations present on the classloader.
And the desire is to have a way to pick the most appropriate client for the specific code to use.

But that brings up other questions ...

• do we really want such a heavy concept be loaded multiple times across different libraries?
• is the "new client for each call to ContainerProvider" still valid?
• what if the higher level application knows of proxy requirements, ssl/tls requirements, and other details that belong to all instances of the WebSocket client? Can we solve for this at the same time?
• what if the application accesses ContainerProvider from a server environment, should it return the singleton client from the ServerContainer instead? (the one that's already configured appropriately for that environment)

[rmannibucau]

Hi everyone,

I think it is either @priority (if available) or useless (if you know which container you want you do new MyContainer(), no?).
The loading should likely be:

0. if the provider system property is set use this provider
1. if there is a single impl -> use it
2. if there are 2 impl -> sort them accordingly @priority (default 1000), if two are equals, sort by class name (to be deterministic) or fail (not sure)

This would make it close to all EE providers loading logic and not custom to a single spec.

Side note: indeed spec impl can cache the provider to use per classloader for perf.

[joakime]

I think it is either @priority (if available) or useless (if you know which container you want you do new MyContainer(), no?).

The use of MyContainer() is supported by many implementation already, yes.
But what if you want to force the specific implementation on other libraries that simply use ContainerProvider.getWebSocketContainer() ?

I should be able to do something like ...

ServerContainer serverBasedClient = (ServerContainer) servletContext.getAttribute("jakarata.websocket.server.ServerContainer");
configure(serverBasedClient);
ContainerProvider.setImplementation(serverBasedClient);

or

FooWebSocketClient client = new FooWebSocketClient();
configure(client);
ContainerProvider.setImplementation(client);

That way, I know that all users of the ContainerProvider.getWebSocketContainer() get what I need them to use.

Where does the @Priority annotation come from? (what library, something unique to websocket I'm sure, as websocket-api intentionally depends on nothing, not even the servlet spec or CDI)

Also, where would the @Priority annotation be placed?
If it's the application code, then there's nothing to do here in the websocket-api level.
I could see a ContainerProvider.getImplementations() API, but that's about it.

I have a different view of this requirement.

The far more useful approach ...

1. If the specific implementation exists, use it everywhere.
2. If the specific implementation does not exist, fail it everywhere.

Priority is not a factor in these use cases.

[rmannibucau]

@joakime you move the problem because as soon as you will expose ContainerProvider.setImplementation(impl) libraries will start to use it so you are back to previous state (it hit several libraries by the past, even EE or microprofile ones). So overall a global setter creates as much issues than it solves it. Also note that you assume you can put code before provider consumer is called which is not always trivial in business/user code so I'd prefer to not go that path.

@Priority comes from jakarta.annotation and can be considered as optional by the implementer (if not present it is ignored, if there use it). Since it is an annotation it does not have to be in the classpath to let applications and impl run.
Priority would be set on the container implementations, enabling libs or users to pick the one they want globally by overriding it (full delegation or inheritance, both will work). I guess default implementations will be priority=1000, lib ones 2000 and user ones 4000 or so.

Indeed if you prefer to add public int priority() { return 1000; } in ContainerProvider it is fine too and strictly equivalent and I can agree maybe better at the end.

Why is it important to make it in the spec? Because if you don't you basically should deprecate ContainerProvider.getWebSocketContainer() since it does not have this pluggability and is almost random right now (in particular between startup depending how your classpath/classloader is created).

Why getImplementations does not help? because it is strictly equivalent to ServiceLoader.load(ContainerProvider.class).stream() or new MyProvider() in consumer code and still require all libs and applications to do the ordering (or find the "new" to do directly or by reflection) - potentially differently when it shouldn't in most cases.

So to summarize: it is key to not delegate the selection responsability to the caller - or if decided, ContainerProvider#getWebSocketContainer must be deprecated but it has some real value and it should be kept IMHO.

I'm not sure I see how your proposal helps since "specific implementation" is what ContainerProvider should resolve and caller code will just call getWebSocketContainer. Take the example of TomEE which will replace the default implementation Tomcat provides, not a single consumer will know about it nor can really know so all stream related or global setter solution wouldn't work well.

So the issue is really the entry point is a static method (to embrace the standalone case) and it does not let "contributors" to do this selection logic in an encapsulated fashion - this is where priority notion helps a lot.

The only solution todo is to wrapp all getWebSocketContainer calls with a custom classloader which fakes the getresources call of ServiceLoader - I guess you see why it is not a "real" solution ;).

edit: @markt-asf staying on java 8 is fine since the new API can be replaced by multiple options (the simplest being to let them all be instantiated - obvious if using priority method option and no more the annotation option - or reimplement the read of these files as before java 6, not crazy is j8 is a big constraint - I can hear that.

[joakime]

@rmannibucau this is great detail, thank you.

Lots to noodle through here. I just am cautious about making the API even messier than it already is.

Example: If I could start from scratch, there would be no streaming support (only partial messages), there would be no encoders or decoders, tracking all sessions would be dropped, there would be a simpler API than the Configurators as defined, client would be more configurable for common scenarios (SSL/TLS, proxy, auth, cookies, etc), CDI would have been involved from the get go to properly delineate the websocket scopes, customized server upgrades would be easier, and proper extension support (both custom extensions, configuring existing extensions, and ability to participate in extension resolution).

But we have to work with what we have.

@Priority comes from jakarta.annotation and can be considered as optional by the implementer (if not present it is ignored, if there use it). Since it is an annotation it does not have to be in the classpath to let applications and impl run.

I'm a bit surprised by this statement, as we (Eclipse Jetty) haven't been able to run without the annotation jar if those annotations are present anywhere in any runtime accessible class (nod to JEP238) on the container or webapp classpaths (for Jetty 9.x). The JPMS layer introduced in Jetty 10 and Jetty 11 also makes this statement extra surprising as that layer pitches a fit if you attempt to load a class with annotations that are not declared in your module-info.java as requires jakarta.annotations (meaning it's non-optional).

[rmannibucau]

@joakime requirement can be made optional (requires static) and java spec requires only available runtime annotations to be loaded in java.lang reflect. Give it a try in a small main:

 @Foo public class Bar {}

 public class Main {
     public static void main(String.... a) {
         System.out.println(List.of(Bar.class.getAnnotations()));
     }
 }

[gregw]

I don't understand the concept of an annotated priority? Surely implementor will just set it to Integer.MAX_VALUE to encourage usage of their implementation!

…

On Wed, 28 Apr 2021, 06:08 Romain Manni-Bucau, ***@***.***> wrote: @joakime <https://github.com/joakime> requirement can be made optional (requires static) and java spec requires only available runtime annotations to be loaded in java.lang reflect. Give it a try in a small main: @foo public class Bar {} public class Main { public static void main(String.... a) { System.out.println(List.of(Bar.class.getAnnotations())); } } — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#365 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAARJLMQ2MJ2FDBNJL7FJ2TTK4KURANCNFSM43TNFBBA> .

[rmannibucau]

@gregw maybe end users but containers and libraries will not to enable users to override it, it is what happens in practise otherwise users report it as a bug. Now happy to get another working solution too but as of today only a sorting works and priority is the most trivial and common solution AFAIK.

[markt-asf]

The @Priority solution seems very geared to the TomEE use case where there is a requirement to override an implementation in a dependency with a local implementation. It works because the @Priority value of the local implementation can be controlled. It doesn't work for the case where there are two implementations provided via dependencies and the container (or the user) wants to pick one.

I'll note that it should be possible to solve the TomEE use case simply by not including the Tomcat WebSocket implementation as it is packaged in a separate JAR for both the standard and embedded distributions.

I wrote various iterations of my analysis of various scenarios but it comes down to containers/users need a way to control the result of ContainerProvider.getWebSocketContainer(). In some scenarios, there are existing ways to work-around this but there are always edge cases that fail.

My current thinking is that a new ContainerProvider.setWebSocketContainer() method that is backed by a per-class loader store combined with a clear direction in the specification that libraries MUST NOT use this method would meet the requirement. (Libraries that wish to depend on a specific implementation should a) instantiate that implementation directly and b) provide a way for users of the library to specify an alternative implementation and/or a way to select an implementation). Calling it with null would clear the cache entry, if any, for the current class loader. We'd need to be careful constructing the cache to avoid class loader memory leaks.

ContainerProvider.getWebSocketContainer() would first look in the cache using the current class-loader as a key, then work up the class loader hierarchy lookup up each class loader in the cache and then use the existing selection process, stopping as soon as a valid implementation is found and then populating cache for the current class loader with that implementation.