Re: (tomcat) branch main updated: Minor review updates

On Mon, Jul 6, 2026 at 11:09 AM <[email protected]> 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 <[email protected]>
> 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: [email protected]
> For additional commands, e-mail: [email protected]
>