paulrutter commented on PR #552: URL: https://github.com/apache/felix-dev/pull/552#issuecomment-5530800149
You're right and my earlier statement was wrong — apologies. `PlurlImpl.install()` returns early when `checkPlurlProtocol()` finds an installed plurl, and `Plurl.add(..)` goes through the `plurl:` channel, so a second copy registers with the live router rather than replacing it. That rendezvous is exactly what plurl provides, and nothing in the Felix install/registration needed changing. I've implemented what I think is needed and split it into two commits so the plurl part can be read on its own: - **[`7f9938c`](https://github.com/apache/felix-dev/commit/7f9938ce896479908213d8c4f7895f9a831df71d)** — the proposed plurl change, touching only the vendored plurl sources - **[`392c522`](https://github.com/apache/felix-dev/commit/392c52234cd7dfaed69346ae8b462655aa143afc)** — the Felix side that uses it With both, `URLHandlersTest.urlHandlersWithClassLoaderIsolation` passes and the framework suite is clean. ### Why protocol-level delegation wouldn't help To answer your question directly: **every Felix framework instance in the JVM uses the same `bundle:` protocol.** Telling plurl "`bundle:` belongs to this factory" doesn't say *which* framework — that's the UUID in the URL host (`<framework-uuid>_<bundleId>`). Equinox encodes the same thing as `<bundleId>.<container.hashCode()>`, but `BundleResourceHandler.openConnection` parses only the bundle id and resolves it against the bound container, so the container part is never consulted for lookup. What's needed is selection that can see the URL. So: ```java public interface PlurlFactory { default boolean shouldHandle(URL url) { return false; } boolean shouldHandle(Class<?> clazz); } ``` consulted in `findFactory` **before** walking the call stack. Defaulting to false means existing factories are unaffected. Felix's implementation is then just a UUID comparison against the host. ### Two things that weren't obvious I had originally proposed only the interface method. It doesn't work on its own, for two reasons I only found by implementing it: **1. Selection happens while the URL is still empty, and the result is cached.** `PlurlRootURLStreamHandler.parseURL` calls `lookupPlurlStreamHandler(u)`, but in `new URL(spec)` the JDK sets only `protocol` before calling `parseURL` — `host` is populated *by* `parseURL` via `setURL`. So at the moment plurl must choose, there is nothing in the URL to inspect. Worse, that choice is then written to `urlToHandler` for the life of the URL, so a `shouldHandle(URL)` hook is never reached for a freshly parsed URL, which is exactly the failing case. The commit therefore only records the handler once the URL is usable for selection: ```java if (!isUsableForSelection(u)) { // still being parsed; only the call stack is available, and caching this // choice would pin the URL to a factory picked before the URL was known return findPlurlStreamHandlerImpl(u); } return urlToHandler.get(u, () -> findPlurlStreamHandlerImpl(u)); ``` `isUsableForSelection` is just a non-empty-host check today; you may want something stricter or protocol-specific. **2. `findFactory` iterates `PlurlFactoryHolder`s, not the factories.** Since `PlurlFactoryHolder implements PlurlFactory`, it inherits the default and answers `false` for every holder, so the hook never reaches a real factory. The holder now delegates `shouldHandle(URL)` the same way it already delegates `shouldHandle(Class)`, including the reflective path for factories compiled against a different copy of the plurl package. My first attempt failed precisely because I missed this. ### Two smaller points - `shouldHandle(URL)` overloads `shouldHandle(Class)`, which makes `shouldHandle(null)` ambiguous — it broke my own test compile. A distinct name would avoid that if you take this upstream. - Separately: when no factory claims a URL, declining looks safer to me than selecting the first factory added. As it stands framework A can be handed a URL naming framework B, and since Equinox resolves only the bundle id against its bound container, if A also has a bundle with that id it returns A's resource rather than an error. Felix's UUID check turns that into a clean failure, which is why we have a test for it and you likely haven't hit it. Happy to leave that as-is if you'd rather keep the current fallback. If the shape looks reasonable I'll raise it as a plurl issue or PR — whichever you prefer. And if you'd rather solve it differently, the Felix commit is easy to redo on top of whatever plurl ends up offering. -- 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]
