paulrutter commented on code in PR #575:
URL: https://github.com/apache/felix-dev/pull/575#discussion_r4279968563
##########
framework/src/main/java/org/apache/felix/framework/URLHandlersStreamHandlerProxy.java:
##########
@@ -303,6 +304,7 @@ protected URLConnection openConnection(URL url) throws
IOException
}
catch (Exception ex)
{
+ rethrowIOException(ex);
Review Comment:
**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:
```java
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:
```java
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.
##########
framework/src/test/java/org/apache/felix/framework/URLHandlersStreamHandlerProxyTest.java:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.felix.framework;
+
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.net.Proxy;
+import java.net.URL;
+import java.net.URLConnection;
+import java.net.URLStreamHandler;
+
+import org.apache.felix.framework.util.SecureAction;
+import org.junit.jupiter.api.Test;
+
+/**
+ * An {@link IOException} thrown by a stream handler invoked through
reflection is propagated
+ * unchanged (FELIX-6861).
+ */
+class URLHandlersStreamHandlerProxyTest
Review Comment:
**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`.
--
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]
