paulrutter commented on PR #552:
URL: https://github.com/apache/felix-dev/pull/552#issuecomment-5500469901
Thanks for sorting the headers — `apache-rat` is satisfied now, which was
blocking the build entirely. I've pushed a substantial rework; summary below,
with one design question at the end.
I refreshed the vendored sources from upstream, so they include the
Apache-2.0 headers from eclipse-osgi-technology/plurl#45 and the fix from
eclipse-osgi-technology/plurl#55. Only the package rename is applied. And `new
PlurlImpl()` rather than the `ServiceLoader`, as you suggested.
### Both of your review points were real defects
- **`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 draft 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.
### Plurl is now the mechanism, not an addition alongside it
- `URLHandlers` no longer swaps the `java.net.URL`/`URLConnection` static
fields, and its singleton is not constructed. I had planned to keep that as a
fallback, but the `catch` in your `SystemBundleActivator` changed my mind: a
fallback keeps the `sun.misc.Unsafe` path alive, which is the thing we're
trying to delete. Felix now behaves as Equinox does — if registration fails it
logs and that framework contributes no URL handlers.
- 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.
- Added a test that starts a framework and asserts registration actually
happened, that the `plurl:` protocol resolves, and that `shouldHandle` doesn't
claim classes outside the framework.
On Linux the framework suite is 121 tests with a single error, which is the
question below.
### 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, each with its own framework. It 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 not loaded by any bundle. There is no bundle frame anywhere on the
stack, so `shouldHandle` cannot attribute it and plurl has nothing to route on.
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.
Instead the framework is resolved per call from the UUID in the URL host, via
`URLHandlers.getFrameworkFromContext(uuid)` — which only sees its own copy's
frameworks. Previously a second copy reflectively called
`registerFrameworkListsForContextSearch` on whichever copy owned the JVM
factory, so UUIDs resolved across class loaders. With plurl there is no "root
`URLHandlers`" to find, so that rendezvous is gone.
I initially assumed Equinox must solve this and Felix was missing something.
Reading `BundleResourceHandler` properly, it's the other way round:
```java
String host = url.getHost(); //
"<bundleId>.<container.hashCode()>"
bundleID = parseBundleIDFromURLHost(host); // parses ONLY the bundle id
Module module = getModule(bundleID); // -> container.getModule(id),
the BOUND container
```
The container is encoded in the host by `createURLHostForBundleID`, but
`openConnection` parses only the bundle id and resolves it against whichever
container the handler was constructed with; the container part is used for
`equals`/`hashCode`/`hostsEqual`/`sameFile`, never to locate the owner. So in
this scenario Equinox would either return the resource with that id **from the
wrong container**, or fail with `URL_NO_BUNDLE_FOUND`. It relies on
`shouldHandle` having routed correctly, with no fallback when attribution is
impossible.
Felix's UUID lookup is a stronger guarantee, which is why there's a test for
it here and no equivalent upstream. So the question isn't how Equinox does it,
but:
**Would plurl consider routing a `URL` to its owning factory, not only a
calling `Class`?**
A `boolean shouldHandle(URL)` on `PlurlStreamHandlerFactory`, consulted when
call stack attribution yields nothing, would let each framework claim its own
URLs by inspecting the host. Both frameworks would then be correct for URLs
re-parsed outside a bundle, and Equinox could tighten its behaviour if it
wanted to.
Without that, the Felix-side options are:
1. Match Equinox — bind the handler to one framework and accept that
cross-framework re-parsing resolves against the wrong one. Converges with
upstream, but drops behaviour Felix has today and means weakening or removing
that test.
2. Keep the guarantee with a Felix-side cross-copy UUID registry — which
reintroduces reflection into exactly the mechanism plurl is meant to replace,
since each class loader has its own copy of both `URLHandlers` and the plurl
classes and there's no shared rendezvous to publish to.
I'd rather not do 1 silently. Happy to raise the API idea as a plurl issue
with a concrete proposal if you think it's reasonable.
--
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]
