You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2026/07/06 09:09:40 UTC
(tomcat) branch main updated: Minor review updates
This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new ecdb83ce32 Minor review updates
ecdb83ce32 is described below
commit ecdb83ce32c6263a4f465982c8de237a5bb9cada
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Jul 6 10:09:11 2026 +0100
Minor review updates
- don't override hashcode() and equals() (the overrides break the
contracts)
- catch Throwable (else ExceptionUtils.handleThrowable() is a NO-OP)
- comment typo
- ensure the trackedStream is returned for multiple getInputStream()
calls
- NO-OP known safe case of FileURLConnection
---
.../tomcat/util/buf/CloseableURLConnection.java | 42 ++++++++++------------
1 file changed, 18 insertions(+), 24 deletions(-)
diff --git a/java/org/apache/tomcat/util/buf/CloseableURLConnection.java b/java/org/apache/tomcat/util/buf/CloseableURLConnection.java
index ea43b0a085..fe54a11cd7 100644
--- a/java/org/apache/tomcat/util/buf/CloseableURLConnection.java
+++ b/java/org/apache/tomcat/util/buf/CloseableURLConnection.java
@@ -135,31 +135,36 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
if (trackedStream != null) {
try {
trackedStream.close();
- } catch (Exception e) {
- ExceptionUtils.handleThrowable(e);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
}
} else if (connection instanceof JarURLConnection) {
try (@SuppressWarnings("unused")
java.util.jar.JarFile jarFile = ((JarURLConnection) connection).getJarFile()) {
// Explicitly close the JarFile to release its native resources.
// As setUseCaches(false) is set, this should not cause side effects on other streams.
- } catch (Exception e) {
- ExceptionUtils.handleThrowable(e);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
}
+ } else if (connection.getClass().getName().equals("sun.net.www.protocol.file.FileURLConnection")) {
+ /*
+ * Internal JDK class so have to check by default name. If a JDK uses another name it will be handled by the
+ * final block.
+ *
+ * NO-OP - known not to open a stream to read metadata
+ */
} else if (!(connection instanceof HttpURLConnection)) {
- // Other cases like FileURLConnection could have used a stream as a side effect,
- // possibly causing file locking.
+ // Other cases could have used a stream as a side effect, possibly causing file locking.
try (@SuppressWarnings("unused") InputStream is = connection.getInputStream()) {
// Explicitly close the InputStream to release its native resources.
- } catch (Exception e) {
- ExceptionUtils.handleThrowable(e);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
}
}
if (connection instanceof HttpURLConnection) {
((HttpURLConnection) connection).disconnect();
}
-
}
@@ -172,7 +177,9 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
*/
@Override
public InputStream getInputStream() throws IOException {
- trackedStream = connection.getInputStream();
+ if (trackedStream == null) {
+ trackedStream = connection.getInputStream();
+ }
return trackedStream;
}
@@ -284,7 +291,7 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
public Permission getPermission() throws IOException {
/*
* This method is deprecated for removal in Java 25. If it isn't overridden the superclass will return {@code
- * java.security.AllPermission} which would be acceptable but, for consistency, it is better to oveerride the
+ * java.security.AllPermission} which would be acceptable but, for consistency, it is better to override the
* method. Calling {@code getPermission()} on the wrapped connection would work until the method is removed.
* Throwing {@code UnsupportedOperationException} works now since Tomcat never calls the method and will
* continue to work once the method is removed.
@@ -393,17 +400,4 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
public Map<String, List<String>> getRequestProperties() {
return connection.getRequestProperties();
}
-
-
- @Override
- public int hashCode() {
- return connection.hashCode();
- }
-
-
- @Override
- public boolean equals(Object obj) {
- return connection.equals(obj);
- }
-
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Minor review updates
Posted by Rémy Maucherat <re...@apache.org>.
On Mon, Jul 6, 2026 at 11:09 AM <ma...@apache.org> wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> markt-asf pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new ecdb83ce32 Minor review updates
> ecdb83ce32 is described below
>
> commit ecdb83ce32c6263a4f465982c8de237a5bb9cada
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Mon Jul 6 10:09:11 2026 +0100
>
> Minor review updates
>
> - don't override hashcode() and equals() (the overrides break the
> contracts)
> - catch Throwable (else ExceptionUtils.handleThrowable() is a NO-OP)
> - comment typo
> - ensure the trackedStream is returned for multiple getInputStream()
> calls
> - NO-OP known safe case of FileURLConnection
> ---
> .../tomcat/util/buf/CloseableURLConnection.java | 42 ++++++++++------------
> 1 file changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/java/org/apache/tomcat/util/buf/CloseableURLConnection.java b/java/org/apache/tomcat/util/buf/CloseableURLConnection.java
> index ea43b0a085..fe54a11cd7 100644
> --- a/java/org/apache/tomcat/util/buf/CloseableURLConnection.java
> +++ b/java/org/apache/tomcat/util/buf/CloseableURLConnection.java
> @@ -135,31 +135,36 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
> if (trackedStream != null) {
> try {
> trackedStream.close();
> - } catch (Exception e) {
> - ExceptionUtils.handleThrowable(e);
> + } catch (Throwable t) {
> + ExceptionUtils.handleThrowable(t);
> }
> } else if (connection instanceof JarURLConnection) {
> try (@SuppressWarnings("unused")
> java.util.jar.JarFile jarFile = ((JarURLConnection) connection).getJarFile()) {
> // Explicitly close the JarFile to release its native resources.
> // As setUseCaches(false) is set, this should not cause side effects on other streams.
> - } catch (Exception e) {
> - ExceptionUtils.handleThrowable(e);
> + } catch (Throwable t) {
> + ExceptionUtils.handleThrowable(t);
> }
> + } else if (connection.getClass().getName().equals("sun.net.www.protocol.file.FileURLConnection")) {
> + /*
> + * Internal JDK class so have to check by default name. If a JDK uses another name it will be handled by the
> + * final block.
> + *
> + * NO-OP - known not to open a stream to read metadata
> + */
Although the LLMs do not like this one very much, this was for TldCache
TldCache had:
- if ("file".equals(url.getProtocol())) {
- // Reading the last modified time opens an input
stream so we
- // need to make sure it is closed again otherwise
the TLD file
- // will be locked until GC runs.
- conn.getInputStream().close();
In FileURLConnection, getLastModified() calls initializeHeaders()
which calls connect() which calls new BufferedInputStream(new
FileInputStream(file.getPath()));
So maybe TldCache is right and Windows locking could occur there,
which is why I used (!(connection instanceof HttpURLConnection)) on
the last else.
I spent hours on that close() method ;) Maybe I still messed up :D
Rémy
> } else if (!(connection instanceof HttpURLConnection)) {
> - // Other cases like FileURLConnection could have used a stream as a side effect,
> - // possibly causing file locking.
> + // Other cases could have used a stream as a side effect, possibly causing file locking.
> try (@SuppressWarnings("unused") InputStream is = connection.getInputStream()) {
> // Explicitly close the InputStream to release its native resources.
> - } catch (Exception e) {
> - ExceptionUtils.handleThrowable(e);
> + } catch (Throwable t) {
> + ExceptionUtils.handleThrowable(t);
> }
> }
>
> if (connection instanceof HttpURLConnection) {
> ((HttpURLConnection) connection).disconnect();
> }
> -
> }
>
>
> @@ -172,7 +177,9 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
> */
> @Override
> public InputStream getInputStream() throws IOException {
> - trackedStream = connection.getInputStream();
> + if (trackedStream == null) {
> + trackedStream = connection.getInputStream();
> + }
> return trackedStream;
> }
>
> @@ -284,7 +291,7 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
> public Permission getPermission() throws IOException {
> /*
> * This method is deprecated for removal in Java 25. If it isn't overridden the superclass will return {@code
> - * java.security.AllPermission} which would be acceptable but, for consistency, it is better to oveerride the
> + * java.security.AllPermission} which would be acceptable but, for consistency, it is better to override the
> * method. Calling {@code getPermission()} on the wrapped connection would work until the method is removed.
> * Throwing {@code UnsupportedOperationException} works now since Tomcat never calls the method and will
> * continue to work once the method is removed.
> @@ -393,17 +400,4 @@ public final class CloseableURLConnection extends URLConnection implements AutoC
> public Map<String, List<String>> getRequestProperties() {
> return connection.getRequestProperties();
> }
> -
> -
> - @Override
> - public int hashCode() {
> - return connection.hashCode();
> - }
> -
> -
> - @Override
> - public boolean equals(Object obj) {
> - return connection.equals(obj);
> - }
> -
> }
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org