paulrutter commented on PR #552:
URL: https://github.com/apache/felix-dev/pull/552#issuecomment-5500355448
Thanks — the Apache-2.0 headers unblocked this, and `apache-rat` is happy
now. I've pushed a substantial rework, and I have one design question at the
end I'd value your view on.
Both of your review points were real defects, not just omissions:
- **`Plurl.install(..)` was never called.** Since the static `Plurl.add(..)`
helpers operate through a `plurl:` URL, without installing the router first
they fail with *unknown protocol: plurl*. Nothing in the prototype exercised
the adapter, so it compiled and the tests passed while being dead code.
- **`shouldHandle` was too loose.** It only checked that a class came from
*some* Felix bundle class loader, so with two frameworks in one JVM either
could answer for the other's bundles. It now also requires the owning framework
to match, as your `EquinoxBundle`/container check does.
Beyond that, plurl is now the mechanism rather than something bolted
alongside, following Equinox:
- `URLHandlers` no longer swaps the `java.net.URL`/`URLConnection` static
fields, and its singleton is not constructed. I had considered keeping that as
a fallback, but your `catch` in `SystemBundleActivator` convinced me otherwise
— a fallback keeps the `sun.misc.Unsafe` path alive, which is the thing we're
trying to delete.
- Removing the swap turned out to be *necessary*, not just tidy: with
`URLHandlers` taking the singletons first, plurl found them occupied and
required `--add-opens java.base/java.net=ALL-UNNAMED`. Installing into a clean
JVM uses the supported API and needs no flag.
- `URLHandlersBundleStreamHandler` and `URLHandlersStreamHandlerProxy` now
extend `PlurlStreamHandlerBase`, mirroring your changes to
`BundleResourceHandler` and `URLStreamHandlerProxy`.
- The router is installed once per JVM and reference counted — uninstalling
per framework tore it down while another framework was still registered.
- `new PlurlImpl()` as you suggested, rather than the `ServiceLoader`.
The framework suite on JDK 25 is at 121 tests, 9 failures, 8 of which are
pre-existing platform-specific ones unrelated to this. There is one left, and
it's a design question rather than a bug:
### Routing a URL re-parsed outside any bundle
`URLHandlersTest.urlHandlersWithClassLoaderIsolation` loads a second copy of
the whole framework (and therefore of the vendored plurl classes) in a separate
class loader, and each copy starts its own framework. The test then does the
equivalent of:
```java
URL url = bundle.getEntry("...");
new URL(url.toExternalForm()).openStream(); // called from a plain test
class
```
That second `new URL(..)` is re-parsed **without** an explicit handler, from
a class that is not loaded by any bundle. So `shouldHandle` cannot attribute it
to a framework and plurl has nothing to route on.
Falling through, the `bundle:` handler has to resolve the framework itself
from the UUID in the URL's host. It can only see its own copy's frameworks, so
when the URL belongs to the framework in the other class loader the lookup
fails. Previously `URLHandlers` bridged this: a second copy reflectively called
`registerFrameworkListsForContextSearch` on whichever copy owned the JVM
factory, so a UUID could be resolved across class loaders. With plurl there is
no "root `URLHandlers`" to find, so that rendezvous is gone.
Note I deliberately do **not** bind the `bundle:` handler to one framework
the way you bind yours to the container: the JVM caches one handler per
protocol, so a pinned handler also gets used for another framework's `bundle:`
URLs and then fails to resolve. That's what forced the UUID lookup.
So: **how should a URL re-parsed from outside any bundle be routed to the
owning framework, when several copies of plurl are live in different class
loaders?** Options I can see:
1. plurl gains a way to route a *URL* (not just a calling class), or to
enumerate registered factories, so the owning factory can be found.
2. Felix keeps its own cross-copy UUID registry — but that reintroduces
reflection into the mechanism plurl is meant to replace.
3. It's accepted as out of scope, and the test's expectation changes.
Does Equinox have an equivalent case? I suspect it doesn't hit this because
its callers are bundle classes that plurl can attribute, so the question may
not have come up. If you think (1) is reasonable I'm happy to raise it upstream
with a concrete API suggestion.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
