Portable SPI for automatically registering Client Features

[johnament]

Presently there is no way for a Client's Feature to be registered automatically. note that the spec doesn't really different the Feature interface between Client & Server runtimes. Based on on-list discussion, it seems like there is some notion of automatically registered Client & Server providers, based on what's available. I suspect what most implementations do is:

• Bake it into their core runtime OpenLiberty for example
• Provide vendor specific integration patterns (see the OpenLiberty example, RestEasy Example (Jersey has this pattern as well, but since their repo is moving don't want to create a bad link)

Therefore, I would like to request that there be some kind of SPI that allows a Client to be configured in a platform agnostic way, so that I don't need to write integration code for RestEasy, Jersey and CXF separately (plus whatever differences may exist in actual server runtimes). The approach RestEasy takes makes a lot of sense, but doesn't give a way to separate Client & Server runtimes.

[mkarg]

@andymc12 @asoldano @jansupol Asking for vendor comments. :-)

[andymc12]

Assuming that we can clear the IP hurdles with pulling in MicroProfile code here, I would propose that we do something similar to MP's RestClientBuilderListener: https://github.com/eclipse/microprofile-rest-client/pull/80

This would allow developers to provide a META-INF/services/javax.ws.rs.ext.ClientBuilderListener file that references a class implementing a new ClientBuilderListener interface - something like this:

public interface ClientBuilderListener {
    void onNewBuilder(ClientBuilder builder);
}

The implementing classes could then register features or providers, set initial properties, etc. before the ClientBuilder.newBuilder() method returns.

[chkal]

Not sure if this is exactly the same issue, but I spent quite some time get automatic feature registration on the server side working for the JSR 371 reference implementation Ozark. My findings:

• For Jersey there is a special SPI called AutoDiscoverable. This is basically just a JAX-RS Feature which is processed even if the user manually specifies components via Application.getClasses().
• RESTEasy allows to register features via a META-INF/services/javax.ws.rs.ext.Providers file which works just like the JDK's ServiceLoader API. This also allows to register features even if the user implemented Application.getClasses().

I'm mentioning all this because maybe we should have a unified way to automatically register both client and server side features.

[asoldano]

@mkarg , RESTEasy behavior has properly been described by @chkal and @johnament . This said, I'd be fine with a common spec way of registering client features.

[johnament]

@chkal only confusion is why you're mentioning Application.getClasses() in the notion of a client. AFAIK, there is no relationship between client and server in this regard (but maybe there should be?)

[chkal]

@johnament I just mentioned it because I was describing my finding about feature detection on the server side and whether you implement Application.getClasses() or not changes this behavior. However, it would be great if feature detection would work the same way on the client and the server.

[andymc12]

@johnament @chkal What if we modified the MP Rest Client RestClientListener approach to be something like a ConfigurableListener. We could still use the ServiceLoader pattern for the auto discovery/registration, but now the interface might look like this:

public interface ConfigurableListener {
    void onNewConfigurable(Configurable configurable, RuntimeType runtimeType);
}

Then we could use the passed-in Configurable to configure both the client (during a ClientBuilder.newBuilder() call) or the server (before invoking the Application.getClasses(), Application.getSingletons(), etc method).

Would that work?

[spericas]

@johnament Jumping a bit late into the discussion, but reading your original post made me wonder: what is the use case from which the automatic registration requirement was derived?

Client instances are (manually) configurable and, as explained in the Javadoc, are typically heavyweight so the number of instances should ideally be limited. The need for automatic registration seems to suggest that many instances are created at different locations. Perhaps I'm completely missing the point here :-)

[andymc12]

@spericas I think the main use case is third party products (like MicroProfile OpenTracing, Metrics, etc.) that want to plug-in to JAX-RS clients. Today, when a third party utility like Metrics wants to track how often a particular URI is invoked, it has to find a vendor-specific API to register itself - or else the user must explicitly register the provider.

In many scenarios, you may have a developer that write a JAX-RS client that may not know the environment that his client code will be hosted in. Some environments may have metrics-tracking plugins; others might have OpenTracing enabled; others might want just want to audit all requests/responses, etc. So this feature is necessary to easily enable these third parties to plug in their providers when JAX-RS clients are created.

As for the heavyweight nature of building multiple clients, I agree. We may want to provide some sort of best practice guide to indicate that users ought to avoid repeated calls to newBuilder(), and cache and re-use client objects at a lower level (WebTarget or Invocation.Builder if possible).

[mkarg]

Sorry for chiming in late. Maybe I missed the point, by why can't we simply do something like ClientBuilder.autoRegisterFeatures()... which simply would find all Features on the classpath and those?

[spericas]

[+1] I generally agree with using the service loader mechanism for this auto config feature

[-1] Using a class scanning mechanism would be very time consuming, especially in a server environment

On Sep 13, 2018, at 5:49 PM, Markus KARG notifications@github.com wrote:

Sorry for chiming in late. Maybe I missed the point, by why can't we simply do something like ClientBuilder.autoRegisterFeatures()... which simply would find all Features on the classpath and those?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse-ee4j/jaxrs-api/issues/596#issuecomment-421164455, or mute the thread https://github.com/notifications/unsubscribe-auth/AFU9NxGE8-K7Au5a-2oo5iFLZL8Eo0N1ks5uatLrgaJpZM4ScPN_.

[andymc12]

All, I know that @johnament opened this issue, but after chatting with @pavolloffay from the MP Open Tracing community, it seems like Open Tracing would benefit from a listener being invoked on creation of a new Client rather than a new ClientBuilder. The main reason is that OT needs to manipulate the executor service specified on the ClientBuilder. If the user then sets a new executor service, then the OT changes are lost. If the listener is invoked on the client object, then the executor service cannot be changed for that client and OT will work as designed.

In my PR #658, I created a new ClientBuilderListener - one advantage is that it is invoked by the ClientBuilder API class so implementors basically get this feature for free. If we were to change this to a ClientListener, the implementors would need to find and invoke the listeners themselves. Performance is also a consideration - I still think most users will cache and re-use clients, but it may still be a factor. What do you all think?

[spericas]

@andymc12 In theory, multiple (but not too many) Client's should be created from a single ClientBuilder. Moving this rather time consuming logic to the creation of each Client seems like a step back to me.

Perhaps someone could explain the OT use case in a bit more detail. Who's creating the ClientBuilder? Could it be addressed by ordering the listeners? (Not a suggestion, simply trying to understand the use case).

There are already many use cases in JAX-RS where users can potentially affect the behavior of a framework. For example, by injecting higher priority filters or interceptors. The fact that this is possible is not considered be a "bug" but a "feature".

[pavolloffay]

@spericas The main concern is around overriding executor service. Tracing integration can consist of these parts:

1. propagate a context (threadlocals) from parent thread to filters
2. filter which is invoked per request
3. (auto) registration/installation of all above

The listener on Client.onBuild() helps with 1. and 3. The listener ClientBuilder.onCreate() also solves the problem but if user calls ClientBuilder.executorService it breaks the integration.

[spericas]

@pavolloffay Is it possible for the OT implementation to detect the condition and abort with an explanation of the problem? As I said above, this is not a new condition, many JAX-RS integrations can be affected by users updating the processing pipeline in unexpected ways. As long as frameworks can detect the problem and inform the user, I don't see a problem.

[pavolloffay]

In this case no. There is now way to know if context is not-propagated because somebody has overridden ExecutorService or because there wasn't context.

This is very ugly issue. Broken traces are hard to detect!

JAX-RS integrations can be affected by users updating the processing pipeline in unexpected ways

Tracing can be also e.g. priority on the filter, and that is ok and expected. But this issue with executor service is nasty and It would be a major improvement to provide an API in JAX-RS which allows context propagation.

[spericas]

Ok, so let's say that we move the listeners to Client from ClientBuilder and someone creates a ClientBuilder and sets a different ExecutorService. How would the OT integration detect/rectify this condition when an instance of Client is created from that ClientBuilder?

[pavolloffay]

This is a very good point. if the ClientBuilder Implementation provides a getter on the executor service it can be done otherwise OT listener overrides it. It's not ideal we should be a way to reliably propagate the context. #668

[spericas]

What if the ExecutorService in use was accessible from the ClientRequestContext available in the filter? Could that be used together with the proposed ClientBuilderListener?

[pavolloffay]

so ClientRequestContext.getExecutorService(). I don't think so it would solve the problem. The filter can be already invoked on different thread so we are not able to get context stored in thread locals from parent thread.

[spericas]

I meant as a way to detect if it the executor had been replaced by the user. In any case, if you have a proposal that works, let's review it.

[pavolloffay]

@andymc12 in https://github.com/eclipse-ee4j/jaxrs-api/issues/596#issuecomment-423335514 proposed something. This also requires adding getter on executorService on the client builder.

Other proposal is might be to create callbacks (before, at, after) when context is switched to different thread.

[mkarg]

I do not understand why you want to use threadlocals instead of simply tagging a request using its properties bundle?

[pavolloffay]

Tracers use so-called SopeManagers to propagate context between multiple instrumentations (e.g. server filter and client filter) in order to correlate spans.

Passing context/span manually is inconvenient especially in languages with thread locals. We cannot ask developers to pass the context manually around.

[mkarg]

Neither can you expect that all vendors of all libraries and applications on earth are forwarding the threadlocals from one thread to the next when using complex thread interactions internally. What will you do in case the application does not pass just a thread pool but actually an essential application-specific functionality to executorService? This is pretty valid, and your executor service replacement would simply screw the application then!

[mkarg]

I wonder if it would make sense to provide a common Generic JAX-RS Extensions SPI instead of a client-only solution? This could work the same way on all platforms (EJB, Servlet, Java SE) in a technical identical way for JAX-RS clients and JAX-RS servers. It should base on ServiceLoader with one unique interface, e. g. javax.ws.rs.ext.Extension, and JAX-RS will then callback into implementations of Extension at specific events, like:

• onBoot
• onShutdown
• onNewClient
• etc.

It wold be a more general approach to external extensibility and maybe opens a lot of possible use cases besides the one or two we had in mind before. Also it prevents "API cluttering" as it would be a central solution for all kinds of extensions.

I would actually love to add that ontop of Java SE Bootstrapping so auto-detection on compression filters and such stuff would be as easy as just adding a maven dependency on an implementing extension module!

Moreover it could help us refactoring features out of JAX-RS core specification into separate addendums (modules) in separate jars / jmods that programmers can add on demand. For example, SSE, JSON, JAXB, etc. could be refactored as JAX-RS Extension Modules that way.

[pavolloffay]

Neither can you expect that all vendors of all libraries and applications on earth are forwarding the threadlocals from one thread to the next when using complex thread interactions internally.

This is not about propagating the context between all interactions but rather making sure that only providers in JAX-RS can get context from caller thread. This is very important for distributed tracing integration and I guess it's been well described here. Here is an example how is it done in MP rest client https://github.com/eclipse/microprofile-rest-client/blob/master/api/src/main/java/org/eclipse/microprofile/rest/client/ext/AsyncInvocationInterceptor.java.

[chkal]

@mkarg While I actually love the idea of having some kind of unified extension API for both the server and the client side, I wonder if there isn't some weird overlapping (at least conceptually) with existing mechanisms like Feature and DynamicFeature. Just thinking out loudly here.

[mkarg]

@chkal The difference between Extension and Feature / DynamicFeature is that the latter are API while the first one is SPI. So for example, that "Auto-Detection" that some platforms provide would be an extension, and that extension would provide the API features. The existing overlap is there because it was not invented earlier. We should take the chance now so we can clearly remove the sections upon "how to load" from the API description and move it to a new SPI section.

[spericas]

@mkarg IMO, extensions like these are more elegantly implemented using CDI, for example, using events. Since CDI is on our roadmap for 3.0, I don't think we should go as far as adding a whole new SPI for this purpose at this time.

[mkarg]

@spericas Agreed, but in turn this means it also makes no sense to provide new SPIs at all - including the one this issue talks about, and the one Andy drafted.

[chkal]

Good point. I really like the CDI extension model. And I would love to see something similar for JAX-RS.

And I agree with @mkarg. If we want to define some kind of extension model based on CDI in JAX-RS 3.0, it would be weird to define a new SPI using the JDK's ServiceLoader today.

[arjantijms]

@chkal

As I think you encountered for MVC 1.0, there's a similar issue with features on the server side. Features can only be registered via an annotation, but if the MVC jar is on the server itself (as opposed to being in the war), it won't be scanned for annotations.

[chkal]

@arjantijms Yes, that's why the MVC RI registers JAX-RS providers via Jersey's ForcedAutoDiscoverable SPI and CDI beans via a custom CDI extension which registers the required types. So this is working fine, even if the MVC RI is provided as a Glassfish module.

[spericas]

@chkal @mkarg Although I agree that we should not introduce a full-blown SPI at this point and wait for a CDI-based alternative down the line, I also believe that Tracing is becoming an increasingly important use case with microservices. Thus, if there's a simple extension that we can introduce for that purpose now, I would support that as well.

[arjantijms]

@chkal

Yes, that's why the MVC RI registers JAX-RS providers via Jersey's ForcedAutoDiscoverable

I actually had the exact same problem. I implemented MicroProfile JWT, which is 99.999% using standard Java EE APIs. Only to registering a necessary JAX-RS Feature for when the jar is part of GlassFish or Payara I had to use that.

Hmmm, I wonder what the best way forward is for standardising ForcedAutoDiscoverable, especially in the context of the Client Features as well.

[chkal]

I actually had the exact same problem. I implemented MicroProfile JWT, which is 99.999% using standard Java EE APIs. Only to registering a necessary JAX-RS Feature for when the jar is part of GlassFish or Payara I had to use that.

I agree. The bootstrapping and registration of providers was one of the major aspects for the MVC RI for which implementation specific solutions were required.

Hmmm, I wonder what the best way forward is for standardising ForcedAutoDiscoverable, especially in the context of the Client Features as well.

I agree that we should think carefully about how the current situation can be improved and how we want to handle new SPIs. Both for the server and for the client.

My 2 cents: Although I really like CDI and the CDI extension model, I also really like ServiceLoader especially for its simplicity and because it works in almost all environments. So I would also be fine with building some generic extension model on top of ServiceLoader.

[mkarg]

So we're back where we started: We need an SPI Extension interface which essentially uses ServiceLoader to enforce auto discoverable, and we do not want to wait until CDI is here. :-)

[chkal]

So we're back where we started

Maybe! This is just my opinion. I would love to hear other thoughts about it.

[mkarg]

I am currently preparing a WIP-PR simply by refactoring Jersey's ForcedAutoDiscoverable, so maybe soon we have real code to discuss. :-)

Unfortunately this process is stuck a bit due to Jersey's rather slow adoption of JAX-RS 2.1.2 and 2.2-SNAPSHOT, which makes further process a bit problematic.