ppkarwasz commented on code in PR #4127:
URL: https://github.com/apache/logging-log4j2/pull/4127#discussion_r3401718525
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationSource.java:
##########
@@ -359,24 +360,44 @@ public String toString() {
try {
final File file = FileUtils.fileFromUri(url.toURI());
final URLConnection urlConnection =
UrlConnectionFactory.createConnection(url);
+ boolean success = false;
+ InputStream openedStream = null;
try {
+ final ConfigurationSource source;
if (file != null) {
- return new
ConfigurationSource(urlConnection.getInputStream(),
FileUtils.fileFromUri(url.toURI()));
+ openedStream = urlConnection.getInputStream();
+ source = new ConfigurationSource(openedStream, file);
} else if (urlConnection instanceof JarURLConnection) {
// Work around
https://bugs.openjdk.java.net/browse/JDK-6956385.
URL jarFileUrl = ((JarURLConnection)
urlConnection).getJarFileURL();
File jarFile = new File(jarFileUrl.getFile());
long lastModified = jarFile.lastModified();
- return new
ConfigurationSource(urlConnection.getInputStream(), url, lastModified);
+ openedStream = urlConnection.getInputStream();
+ source = new ConfigurationSource(openedStream, url,
lastModified);
} else {
- return new ConfigurationSource(
- urlConnection.getInputStream(), url,
urlConnection.getLastModified());
+ openedStream = urlConnection.getInputStream();
+ source = new ConfigurationSource(openedStream, url,
urlConnection.getLastModified());
}
- } catch (FileNotFoundException ex) {
+ success = true;
+ return source;
+ } catch (final FileNotFoundException ex) {
ConfigurationFactory.LOGGER.info("Unable to locate file {},
ignoring.", url.toString());
return null;
+ } finally {
+ if (!success) {
+ if (openedStream != null) {
+ try {
+ openedStream.close();
+ } catch (final IOException ignored) {
+ // Best-effort cleanup
+ }
+ }
+ if (urlConnection instanceof HttpURLConnection) {
+ ((HttpURLConnection) urlConnection).disconnect();
+ }
Review Comment:
Thank you for your PR!
This `finally` clause and `success` flag complicate the code more than
necessary, and the `openedStream != null` branch looks like dead code:
`openedStream` is only assigned by `getInputStream()`, so if that call throws
an `IOException` the variable is still `null`, and if it succeeds we go
straight to building the `ConfigurationSource` and returning (the constructors
don't throw). There is no path where `openedStream` is non-null and `success`
is `false`, so `openedStream.close()` is never actually reached.
Your real goal here is to call `disconnect()` on the `HttpURLConnection` on
the error path, right? If so, I'd suggest splitting the nested `try...catch`
into two sequential ones:
1. The first assigns `file` and `urlConnection`. Nothing has been opened
yet, so it needs no cleanup (it just catches `URISyntaxException` and
`IOException` and logs).
2. The second calls `getInputStream()` and builds the source, and calls
`disconnect()` **only** in the `IOException` catch. No `success` flag, and no
`openedStream.close()`.
A single `catch (IOException)` in the second block covers both the generic
case and `FileNotFoundException` (which is what a missing file, or an HTTP
404/410, surfaces as), so the two can share one handler, but we can have
different log messages.
One small thing to fold in: read `urlConnection.getLastModified()` *before*
`getInputStream()` in the last branch, so the stream is only opened once
everything that could fail has already succeeded.
--
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]
