On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with two > additional commits since the last revision: > > - typo blank linke correction > - revise the update I parsed the finalize() code one more time. The sending close_notify may be not an expected part of the finalize() implementation. The finalize() calls the close() method. If the socket is layered over a preexisting socket, the preexisting socket is closed by calling it close() method: ` self.close(); ` Otherwise, the SSLSocket.close() method will be called: ` super.close(); ` See the BaseSSLSocketImpl.close() method: @Override public void close() throws IOException { if (self == this) { super.close(); } else { self.close(); } } For layered over socket case, if the preexisting socket is not an SSLSocket object(which is the common case), no close_notify will be sent off course. If the preexisting socket is an SSLSocket object (which may be not common), the SSLSocketImpl.close() will be called and the close_notify could be sent. For non-layered over sockets, as super.close() is called, there is no close_notify delivered during the finalizing. Based on the cases above, the close_notify delivery may be not an expected behavior during finalization in practice. I would like to remove the finalize() method without adding a cleaner, as shown in the current PR. But if you read the code and behavior differently , it's acceptable to me to clean up the preexisting socket, by adding a cleaner like: BaseSSLSocketImpl(Socket socket) { super(); this.self = socket; this.consumedInput = null; + CleanerFactory.cleaner().register(this, self::close); } Please let me know your preference. ------------- PR: https://git.openjdk.java.net/jdk/pull/8065