On Mon, 3 Feb 2025 19:13:10 GMT, Brent Christian <bchri...@openjdk.org> wrote:
>> 3 finalizers in RMI code can be removed, as they do not perform meaningful >> cleanup. >> >> **`jdk.naming.rmi/share/classes/com/sun/jndi/rmi/registry/RegistryContext`** >> >> `RegistryContext.finalize()` just calls `close()`. The `close()` method does >> not perform any cleanup per se, but rather "helps the garbage collector" by >> setting `environment` and `registry` to `null`. >> >> >> **`jdk.naming.rmi/share/classes/com/sun/jndi/rmi/registry/RegistryContext.BindingEnumeration`** >> >> `BindingEnumeration.finalize()` simply calls `close()` on the `ctx` field, >> itself a `RegistryContext` (and `close()` just "helps the GC.") >> >> >> **`src/java.rmi/share/classes/sun/rmi/log/LogInputStream`** >> >> `LogInputStream` tracks its length with an int field, `length`. If `length` >> ever becomes == 0, `LogInputStream`'s methods will return without doing >> anything. >> >> The finalizer calls `close()`, which just sets length = 0. >> >> By the time a `LogInputStream` becomes unreachable and is finalized, it's a >> moot point whether length == 0, as no more methods can be called. >> If anything, this finalizer could cause a bug. If a `LogInputStream` were to >> became unreachable while a method were still running, the finalizer could >> set the length to 0 while method code is still running and expecting a >> length != 0. >> It's possible that there is a very long standing bug that `close()` should >> be calling `in.close()`, in which case this evaluation will need to be >> revisited. > > Brent Christian has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'master' into rmF10Nnow > - Remove finalizer from RMI LogInputStream > - Remove finalizer from RMI RegistryContext.BindingEnumeration > - Remove finalizer from RMI RegistryContext > It's possible that there is a very long standing bug that close() should be > calling in.close(), in which case this evaluation will need to be revisited. I don't think this is an issue. The only user of LogInputStream appears to be in [ReliableLog](https://github.com/openjdk/jdk/blob/bad39b6d8892ba9b86bc81bf01108a1df617defb/src/java.rmi/share/classes/sun/rmi/log/ReliableLog.java#L754), which creates an input stream, wraps it in a LogInputStream one or more times, and then closes the input stream in a finally-block. Thus LogInputStream isn't responsible for closing the input stream. ------------- PR Comment: https://git.openjdk.org/jdk/pull/23406#issuecomment-2634747157
