Jetty 10 Configuration abstraction

[sbordet]

In Jetty 10, a o.e.j.webapp.Configuration abstraction has been added, but unfortunately it does not play well with JPMS modules.

The problem is that it relies on the ServiceLoader mechanism, but assuming classes are available in the class-path.

For example the Jetty module jetty-jmx defines a Configuration service JmxConfiguration that is actually present in the jetty-webapp module.
The same happens for Jetty module apache-jsp.

In order for the service in jetty-jmx to be discovered by the JPMS module machinery, you would need to add it to the jetty-jmx module-info.java:

module org.eclipse.jetty.jmx {
  ...
  provides org.eclipse.jetty.webapp.Configuration with org.eclipse.jetty.webapp.JmxConfiguration
} 

Unfortunately this means that jetty-jmx now has a hard dependency on jetty-webapp which is not good.

With JPMS, the service implementation must be provided by the JPMS module itself, not by some other JPMS module.

[sbordet]

Trying to strip down the problem.

JPMS module util contains a service interface S and a number of implementations, say A and B.
JPMS module util uses S and provides S with A.

When only util.jar is in the module-path, I would like only service implementation A to be discovered.

But when also B.jar (which is optional) is in module-path, then I would like both A and B to be discovered.

@nicolaiparlog do you have suggestions?

[nipafx]

I have an idea how to make that work, but it would of course be much easier if the jetty-webapp module would provide the service. Why doesn't it?

[sbordet]

@nicolaiparlog because providing that service is optional and enabled only when B.jar is in the classpath.

Think, for example, JMX.

We want some webapp JMX things to be available, but only if also the jetty-jmx.jar is in the module-path.

The jetty-webapp module has a requires static dependency on jetty-jmx.

[nipafx]

I would have to know more about the code to know whether what you describe is really the best approach, but let's assume it is and we need that service in A only when B is present.

My general recommendation is to never use "the thing itself" as a service, but a factory for it, so the factory can make decisions like what you describe. What about this?

interface ConfigurationFactory {
    Optional<Configuration> create(...);
}

public class JmxConfigurationFactory {
    public Optional<Configuration> create(...) {
        // if JMX present, return JmxConfiguration, else return empty()
    }
}

[sbordet]

@nicolaiparlog this should work in both module-path and class-path, so we'll review it and eventually take a stab at implementing "if JMX present" in a way that works for both module-path and class-path.
If you have suggestions about how to implement "if JMX present" in both cases will be great. Thanks!

[gregw]

All,

just to explain a little bit about the why the JmxConfiguration is not in jetty-jmx.

Jetty has a bunch of modules that build up the basics which a way below the level of know what a webapp is:

[INFO] org.eclipse.jetty:jetty-server:jar:10.0.0-SNAPSHOT
[INFO] +- javax.servlet:javax.servlet-api:jar:4.0.0:compile
[INFO] +- org.eclipse.jetty:jetty-http:jar:10.0.0-SNAPSHOT:compile
[INFO] |  \- org.eclipse.jetty:jetty-util:jar:10.0.0-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-io:jar:10.0.0-SNAPSHOT:compile
[INFO] +- org.eclipse.jetty:jetty-jmx:jar:10.0.0-SNAPSHOT:compile (optional) 

Together, those modules can run a HTTP server, which can optionally be instrumented with JMX - all with no knowledge of webapps or their classloader rules about hiding etc.

The in the jetty-webapp module, we introduce the concept of webapps (and their classloader rules etc.) As part of this, we introduce the concept of a Configuration, which is a discovered service that tells the webapp what features are on the containers classpath and thus how the classloader rules should be configured.

Modules that are developed with full knowledge of webapps will have their own FooBarConfiguration implementations. However, for features such as JMX, which can be used independently of webapps, the jetty-webapp module itself provides a JmxConfiguration implementation as it knows about both JMX and Configurations.

Now just because the service loader discovers the JmxConfiguration from the jetty-webapp module, does not mean that: a) jmx has to be available; b) even if available that a webapp has to use it.

I think it should probably be sufficient if we modify JmxConfiguration to have a required class, so that if during ServiceLoader discovery, the JmxConfiguration is unable to be loaded because the jetty-jmx module is not available, then the JmxConfiguration simply is not an available configuration. If jetty-jmx is on the module path, then it should be discover and loaded and available. What am I missing here?

[gregw]

@sbordet isn't it sufficient just to make JmxConfiguration properly dependent on jetty-jmx:

public class JmxConfiguration extends AbstractConfiguration
{
    public JmxConfiguration()
    {
        addDependents(WebXmlConfiguration.class, MetaInfConfiguration.class, WebInfConfiguration.class);
        protectAndExpose(ObjectMBean.class.getPackage().getName()+".");
    }
}

if jetty-jmx is not on the module path, then the JmxConfiguration simply will not load and thus not be available.

[joakime]

jetty-jmx has been an oddball (to me).

currently, the dependencies look like this ...

• jetty-webapp -> jetty-jmx (for annotations)
• jetty-webapp -> custom mbeans -> javax.management
• jetty-jmx (for bindings / implementation) -> javax.management -> (reads annotations and custom mbeans from) -> jetty-webapp

IMO, the annotations should be 1 module, the bindings/implementation/configuration should be another/different module.

[gregw]

@joakime

I'm not following you?

jetty-webapp is not dependent on jetty-jmx for annotations! If fact in 10.0.x, it really isn't dependent on jetty-jmx in any hard sense as there are no actual usages. I'm proposing that we add a usage, but only in the JmxConfiguration which will fail to load (gracefully) if the optional jetty-jmx is not available.

Generally speaking, all modules provide their own MBeans, but these are optional so most modules optionally depend on jetty-jmx. Modules that are aware of jetty-webapp should provide any Configuration needed and it is only modules that are unaware of jetty-webapp that have their configuration provided by jetty-webapp.

I'm not following how jetty-annotation is involved at all here?

[gregw]

@sbordet as @janbartel has just pointed out to me that the META-INF/services file for org.eclipse.jetty.webapp.JmxConfiguration is in jetty-jmx, even though the class is provided by jetty-webapp.

So as well as making JmxConfiguration dependent on a jetty-jmx class, we should move that services entry to jetty-webapp, which is after all the module that is providing org.eclipse.jetty.webapp.JmxConfiguration

[gregw]

I've created PR #2984 to show how I think this can be fixed.

However, it is a little verbose giving and info saying JmxConfiguration can't be loaded if it is not able to be loaded.
It may be better to have an isAvailable() method on Configuration, so the configuration itself can check its optional dependencies and if not present it would not be available without any logging other than debug. This would still generate a warning if a configuration could not be loaded for some other reason.

[nipafx]

So as well as making JmxConfiguration dependent on a jetty-jmx class, we should move that services entry to jetty-webapp, which is after all the module that is providing org.eclipse.jetty.webapp.JmxConfiguration

If this is an option, it is definitely the cleanest solution (at least as far as the JPMS is concerned).

[stale]

This issue has been automatically marked as stale because it has been a full year without activit. It will be closed if no further activity occurs. Thank you for your contributions.

[joakime]

@sbordet is this JPMS concern still true in Jetty 10? I thought we had a successful (and happy) JPMS on Jetty 10 now?

[sbordet]

I would need to review this.

[gregw]

Closing as fixed. @sbordet open a more specific issue if there is one!