On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan <[email protected]> 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
