paulrutter commented on code in PR #575:
URL: https://github.com/apache/felix-dev/pull/575#discussion_r4280178302
##########
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:
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.
##########
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:
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`:
```java
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.
--
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]
