You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@tomee.apache.org by "jungm (via GitHub)" <gi...@apache.org> on 2026/08/01 07:53:46 UTC

[PR] TOMEE-4643 - discover Feature/DynamicFeature from META-INF/services (tomee)

jungm opened a new pull request, #2877:
URL: https://github.com/apache/tomee/pull/2877

Jakarta REST requires `Feature` and `DynamicFeature` implementations declared in `META-INF/services` to be registered through the JDK `ServiceLoader` at deploy time ("Services", Providers / Lifecycle and Environment).

CXF does not implement this lookup itself — it relies on the container to hand it already resolved providers, which is why the CXF TCK run stays green on GlassFish. TomEE assembles its provider list on its own, so features packaged this way never registered.

## Changes

- `CxfRsHttpListener` scans both SPIs while assembling the provider list.
- Service loading is skipped when the `Application` maps `jakarta.ws.rs.loadServices` to `Boolean.FALSE`, as the spec mandates, and the existing `cxf.jaxrs.skip-provider-scanning` property is still honoured.
- Entries are loaded one at a time so a single broken descriptor entry is logged and skipped instead of discarding the remaining providers of that type.
- Discovered instances flow through the existing `providers(...)` path, so the `@ConstrainedTo` and deactivation checks still apply.
- Drops the now-passing TCK exclusion for `jaxrs31/spec/extensions/JAXRSClientIT`.

## Testing

- New `ServiceLoaderProviderDiscoveryTest` (8 tests) exercises discovery through a throwaway `URLClassLoader`, so it does not register these features into other tests in the module. Each assertion was confirmed to fail when the corresponding production change is reverted.
- `ee.jakarta.tck.ws.rs.jaxrs31.spec.extensions.JAXRSClientIT` passes 2/2 unexcluded against a rebuilt TomEE plus distribution; the wider `jaxrs31.**` slice passes 4/4.
- Full `openejb-cxf-rs` suite: 123 tests, the only failure being `EJBExceptionMapperTest.security`, which fails identically on a clean checkout of `main`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] TOMEE-4643 - discover Feature/DynamicFeature from META-INF/services (tomee)

Posted by "jungm (via GitHub)" <gi...@apache.org>.

jungm commented on PR #2877:
URL: https://github.com/apache/tomee/pull/2877#issuecomment-5213000869

   its flaky, sseBroadcastTest starts 5 SSE clients on threads, waits a fixed 2.5s for them to connect, broadcasts 7 messages, then waits only on the first client before asserting all 5 received exactly 8 events. Clients could not be connecting in time or still be processing evens on a crowded machine
   
   Also seems to have failed in other runs I did here and there


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] TOMEE-4643 - discover Feature/DynamicFeature from META-INF/services (tomee)

Posted by "rzo1 (via GitHub)" <gi...@apache.org>.

rzo1 commented on PR #2877:
URL: https://github.com/apache/tomee/pull/2877#issuecomment-5177779539

   @jungm Can you check on ee.jakarta.tck.ws.rs.jaxrs21.ee.sse.ssebroadcaster.JAXRSClientIT.sseBroadcastTest ? Is this a regression?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] TOMEE-4643 - discover Feature/DynamicFeature from META-INF/services (tomee)

Posted by "jungm (via GitHub)" <gi...@apache.org>.

jungm commented on PR #2877:
URL: https://github.com/apache/tomee/pull/2877#issuecomment-5150522483

   https://ci-builds.apache.org/job/Tomee/view/tomee-11.x/job/pull-request-manual-with-tck/57/


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] TOMEE-4643 - discover Feature/DynamicFeature from META-INF/services (tomee)

Posted by "jungm (via GitHub)" <gi...@apache.org>.

jungm commented on PR #2877:
URL: https://github.com/apache/tomee/pull/2877#issuecomment-5178243543

   @rzo1 think its flaky but will check


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] TOMEE-4643 - discover Feature/DynamicFeature from META-INF/services (tomee)

Posted by "rzo1 (via GitHub)" <gi...@apache.org>.

rzo1 commented on PR #2877:
URL: https://github.com/apache/tomee/pull/2877#issuecomment-5318833333

Currently short on time, so AI review only: I ran an adversarial review pass over this PR, followed by a second pass whose job was to *refute* the first one's findings against the actual code. Everything below survived that second pass. Take it as input, not as a verdict — I have not run the build.

**Overall: the mechanism looks sound.** `addServiceLoaderProviders` is wired into `configureFactory` after the `application.getClasses()` provider loop, so the `alreadyRegistered` check de-dupes against both the `Class` entries and the singleton instances `RESTService` puts into `additionalProviders`. The local `ArrayList` copy means the caller's collection is not mutated. The classloader story holds: `configureFactory` runs with the TCCL set to `CxfUtil.initBusLoader()` and `CxfContainerClassLoader.findResources` delegates to the per-thread webapp loader, so webapp descriptors are visible. `InternalApplication.getProperties()` delegates to the original application, so the `jakarta.ws.rs.loadServices` opt-out is not lost by the wrapper. The hand-rolled `hasNext()/next()` error loop was reproduced standalone against JDK 17 `ServiceLoader` — it recovers from missing classes, throwing constructors and missing no-arg constructors, and terminates in every case.

### 1. `@ConstrainedTo(CLIENT)` features are rejected with an exception instead of being skipped (minor)

`CxfRsHttpListener.java` — the discovered instances are appended to `additionalProviders` and therefore flow into `providers(...)`, whose instance branch calls `isNotServerProvider(o.getClass())`. That method does not merely filter: with `openejb.jaxrs.fail-on-constrainedto` defaulting to `true` it *throws* `IllegalArgumentException`.

That check was previously only ever applied to providers the application itself declared, i.e. things the developer opted into. Service loading changes the population being validated: it now includes every `Feature` declared by any jar visible to the webapp classloader. `@ConstrainedTo(RuntimeType.CLIENT)` is precisely the mechanism the spec gives a library to say "do not register me on the server", so the container should skip it, not fail.

Scope check, so this is not overstated: the exception escapes `configureFactory` → `deployApplication` → `RESTService.afterApplicationCreated`, but `ObserverManager.MethodInvocation.invoke` catches `InvocationTargetException`, fires `ObserverFailed` and logs SEVERE without rethrowing — so `Assembler.createApplication` continues and the application still deploys. What is lost is the REST publication of that app (and of any webapp later in the loop), with a SEVERE stack trace. Also, no artifact in a full local `~/.m2` scan ships a `META-INF/services/jakarta.ws.rs.core.Feature` descriptor at all today, so this is a forward-looking robustness gap rather than an observable regression.

Suggested fix: filter `@ConstrainedTo` inside `addServiceLoaderProviders` and drop the entry with a warning, or pass a flag so `isNotServerProvider` never throws for service-loaded entries. `FAIL_ON_CONSTRAINED_TO` exists to catch mistakes in the app's own provider set; it should not judge ambient classpath entries.

### 2. `jakarta.ws.rs.loadServices` is only honoured as a `Boolean`, not as `"false"` (minor)

`isServiceLoadingEnabled()` uses `!Boolean.FALSE.equals(properties.get(LOAD_SERVICES_PROPERTY))`. `Application.getProperties()` is user-supplied and commonly carries values that originated as strings (config files, init-params, property files merged into the map). `"false"` is not `Boolean.FALSE`, so the opt-out is silently ignored and the scan still runs. Accepting `Boolean.FALSE` or `"false"` (case-insensitive) would make the spec switch behave as users expect.

### 3. The comment and test name overstate the per-entry recovery (nit)

The comment ("a single broken entry must not discard the remaining providers of this type") and `brokenEntryDoesNotDiscardTheOthers` promise more than the JDK allows. Per-entry recovery works for unresolvable class names, missing no-arg constructors and throwing constructors — all three verified. But a malformed *line* makes `ServiceLoader.parse()` abort the whole file (it reads the descriptor eagerly and fails it entirely, dropping even entries listed before the bad line), and since `configs` has already consumed that URL the loop resumes at the next descriptor. The loop is still safe and terminates — this is purely a wording issue. Either soften the comment or add the malformed-line case to the test so the real limit is documented.

### Test coverage

All eight new tests call `addServiceLoaderProviders`/`isServiceLoadingEnabled` in isolation. Nothing exercises the new `if (!ignoreAutoProviders && isServiceLoadingEnabled(application))` wiring in `configureFactory`, nor the `providers(...)` path the discovered instances actually take — which is where finding 1 lives. A test declaring a `@ConstrainedTo(RuntimeType.CLIENT)` feature in a descriptor and asserting the deployment still publishes would pin that invariant.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org