FELIX-6861 Propagate IOExceptions of stream handlers invoked reflectively

[sharanggupta]

Fixes FELIX-6861.

URLHandlersStreamHandlerProxy.openConnection(URL) and openConnection(URL, Proxy) invoke a stream handler that is not a URLStreamHandlerService of the framework class loader (the built-in handler, or a handler from another class loader) through reflection. Method.invoke() wraps an IOException thrown by the handler in an InvocationTargetException, which the generic catch (Exception) then wrapped in an IllegalStateException("Stream handler unavailable due to: null"). So URL.openConnection() failures such as FileNotFoundException escaped catch (IOException) blocks, unlike with a direct invocation of the handler.

A small rethrowIOException() helper now rethrows the handler's IOException (the cause of the InvocationTargetException) on the four reflective paths:

• the reflective invocation in openConnection(URL) and in openConnection(URL, Proxy);
• the extension handler branch of openConnection(URL), which wrapped it in a new IOException without the original type or stack;
• the reflective call of openConnection(URL, Proxy) on a URLStreamHandlerService, which did the same.

Other invocation failures keep their current handling (IllegalStateException or IOException as before).

Test: URLHandlersStreamHandlerProxyTest opens a connection through the proxy with a built-in handler that throws a FileNotFoundException, with and without a Proxy, and checks that the same exception instance is thrown; a handler throwing an UnsupportedOperationException is still reported as an IllegalStateException. On master the first two fail with the IllegalStateException above; with the change all three pass, and mvn verify of the framework module passes locally.

[paulrutter]

Reviewed the diff. The core change is correct and the diagnosis is right: Method.invoke() wraps a handler's IOException in an InvocationTargetException, which slipped past catch (IOException) and became IllegalStateException("Stream handler unavailable due to: null"). Unwrapping the cause is the right fix, and routing all four reflective sites through one helper keeps it consistent. The two sites that previously did if (e instanceof IOException) throw (IOException) e; lose nothing, since rethrowIOException() keeps that check first.

A couple of specifics I checked and found fine:

• InvocationTargetException is already imported, so no import change was needed.
• JUnit 5 and AssertJ are already test dependencies of the framework module and every existing test there uses them, so the new test fits the module's conventions.
• The third test case is a genuine guard: UnsupportedOperationException arrives as an InvocationTargetException whose cause is not an IOException, so rethrowIOException() falls through and the IllegalStateException wrapping is preserved.

Two findings below: one correctness gap on a path the fix does not reach, and one test-coverage gap. Neither blocks the change as far as it goes — the gap is pre-existing, not introduced here.

Interaction with the plurl work

For whoever sequences this: #552 also edits this file, but only to change the base class to PlurlStreamHandlerBase and widen eight methods from protected to public. It leaves the exception handling byte-identical, so this fix is not already covered there. I test-merged this branch into the plurl branch with git merge-tree and it is clean (exit 0, no conflicts) — #552 edits signature lines, this PR edits the catch blocks below them. So this can land on master independently and will flow into the stacked branches on the next sync. Worth re-running the new test after that sync, though, since the proxy will then extend PlurlStreamHandlerBase rather than URLStreamHandler.

[paulrutter]

The URLStreamHandlerService fast path still loses the IOException.

The four reflective sites are fixed, but openConnection(URL) returns early at line 266 before reaching any of them:

if (svc instanceof URLStreamHandlerService)
{
    return ((URLStreamHandlerService) svc).openConnection(url);
}

That looks like a direct call, but svc is frequently a dynamic proxy. When the framework's service object is not an instance of this class loader's URLStreamHandlerService — the "handler from another class loader" case named in the PR description — getStreamHandlerService() wraps it at line 622:

return m_action.createProxy(
        m_action.getClassLoader(URLStreamHandlerService.class),
        new Class[]{URLStreamHandlerService.class},
        new URLHandlersStreamHandlerProxy(service, m_action));

That proxy is instanceof URLStreamHandlerService, so it takes the line 266 branch, which dispatches into invoke() at line 660 — m_action.invokeDirect(...) -> Method.invoke. The handler's IOException becomes an InvocationTargetException, and since ITE is not assignable to IOException, the JDK proxy wraps it again in UndeclaredThrowableException.

I confirmed the wrapping with a standalone reproduction of that exact shape:

thrown: java.lang.reflect.UndeclaredThrowableException
  cause: java.lang.reflect.InvocationTargetException
  caught by 'catch (IOException)'? false

So on this path a FileNotFoundException still escapes callers' catch (IOException) blocks as an unchecked exception — the same FELIX-6861 symptom, just with a different wrapper. The call at line 266 isn't inside a try either, so nothing downgrades it.

This is pre-existing rather than introduced here, so I would not hold the PR for it. But since the description frames the fix as covering handlers "from another class loader", it is worth either handling in invoke() (unwrap the ITE there so the proxy sees a declared IOException) or calling out explicitly as out of scope so it isn't assumed fixed.

[sharanggupta]

Thanks, you're right, and the PR description promised exactly that case. Fixed in 2232120: invoke() now catches the InvocationTargetException and throws its target exception, as a direct call would. The proxy then propagates the declared IOException of openConnection instead of wrapping it in an UndeclaredThrowableException. handlerServiceOfAnotherClassLoaderPropagatesIOException builds the proxy the way getStreamHandlerService() does, with a service that is not a URLStreamHandlerService. It fails with UndeclaredThrowableException before this commit.

[paulrutter]

Verified on 22321201ce — the fix works and both new tests are genuine regression tests rather than ones that would have passed anyway.

check	result
URLHandlersStreamHandlerProxyTest as it stands	5/5 pass
same tests with only invoke() reverted to 04c65f3d3a	handlerServiceOfAnotherClassLoaderPropagatesIOException fails with exactly UndeclaredThrowableException
delete just the rethrowIOException(e) at line 340	only openConnectionWithProxyPropagatesIOExceptionOfHandlerService fails
full framework suite	127 tests, 8 failures — the identical pre-existing deleteDir/BundleCacheTest file-locking set I get on master on Windows, unchanged

The last row is the one I most wanted to see, since invoke() is the invocation handler for every proxied method, not just openConnection. Unwrapping the InvocationTargetException there also changes parseURL and the URLStreamHandlerSetter path, and nothing regressed. Throwing getTargetException() is also consistent for the m_service == null branch, which had the same wrapping.

Thanks for turning it around so quickly.

[paulrutter]

Two of the four changed sites have no test.

rethrowIOException() was added at four places, but the tests reach only two:

site	line	covered
OPEN_CONNECTION.invoke(svc, url)	299	yes - openConnectionPropagatesIOExceptionOfHandler
OPEN_CONNECTION_PROXY.invoke(svc, url, proxy)	346	yes - openConnectionWithProxyPropagatesIOExceptionOfHandler
extension-manager branch (http://felix.extensions:9)	281	no
URLStreamHandlerService + Proxy branch	336	no

Both tests construct the proxy with a plain URLStreamHandler as the built-in, so getStreamHandlerService() returns it, svc instanceof URLStreamHandlerService is false, and control always reaches the same two sites.

The practical risk is low today, but a later refactor could delete either uncovered rethrowIOException() call and the suite would stay green while the regression returns on those paths. The URLStreamHandlerService + Proxy site is the cheaper of the two to cover: a stub implementing URLStreamHandlerService and declaring openConnection(URL, Proxy) (the getMethod lookup at line 332 requires the latter) that throws a FileNotFoundException.

[sharanggupta]

Added openConnectionWithProxyPropagatesIOExceptionOfHandlerService for the URLStreamHandlerService + Proxy branch in 2232120. It uses a stub extending AbstractURLStreamHandlerService with a public openConnection(URL, Proxy) as the built-in handler. The extension-manager branch is still not covered: it needs a framework whose ExtensionManager instance is reachable through the static m_extensionManager field, which this unit test does not set up. If you know an existing test that starts one, I can add a case there.

[paulrutter]

There is no such test, and it is worth knowing why before anyone spends time on one: that branch is unreachable as written, so there is nothing to cover.

Line 277 looks up a static field on ExtensionManager:

m_action.getDeclaredField(ExtensionManager.class, "m_extensionManager", null)

but ExtensionManager declares no field of that name. javap -p on the compiled class lists only:

static final ...ClassPathExtender m_extenderFramework;
static final ...ClassPathExtender m_extenderBoot;
private volatile ...ExtensionManagerRevision m_systemBundleRevision;
private final java.util.List<...ExtensionTuple> m_extensionTuples;

The name m_extensionManager exists only as a private instance field on Felix (Felix.java:119), and getDeclaredField does not search other classes. So the lookup always throws NoSuchFieldException, which lands in the catch (Exception ex) below and becomes throw new IOException(ex.getMessage()) — a URL of the form http://felix.extensions:9 can never return a connection, regardless of whether a framework is running.

So I would leave the branch out of scope here. Your rethrowIOException(ex) call on it is harmless and correct if the lookup is ever repaired; it just cannot be exercised today. That is a separate pre-existing bug from FELIX-6861, and I have not raised a ticket for it — flagging it here so it is on the record for whoever picks it up.

Both findings from my review are addressed as far as this PR is concerned; no further changes needed from me.