Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Wed, 27 Apr 2022 06:55:42 GMT, Xue-Lei Andrew Fan wrote: >> 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 > > ping ... > @XueleiFan, would you please add a note to the bug itself with the > end-result, and a quick note in the place of the finalizer a quick summary of > what we decided. Sure. Notes were added in JBS and the patch. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan 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 Marked as reviewed by wetmore (Reviewer). src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java line 265: > 263: } > 264: > 265: /** Can you please add a quick couple lines here with the technical rationale for removing it, so we don't forget down the road. i.e. There used to be a finalize() here, but decided that was not needed. If users don't properly close the socket... The native resources are handled by the Socket Cleaner. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan 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 Ok, after much back and forth, I'm convinced this is ok. @dfuch commented in another thread: > An application should really close its sockets and not let them get garbage > collected without closing them: this is sloppy. > So brutally closing the underlying TCP connection in that case should be an > acceptable behaviour, and that would be achieved by just removing the > finalize. Thanks for allowing me to look more deeply into this. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 14 Apr 2022 15:37:05 GMT, Daniel Jeliński wrote: > IMO we should not send close_notify in the finalizer. It's the application's > responsibility to send close_notify when it's done with the socket; we should > not pretend that it was closed normally when it was not. @djelinski makes an excellent point which I hadn't really considered. This is an error situation. As the native Socket resources are cleaned/released as needed, a simple removal might actually be appropriate. @XueleiFan I'm going to send you some internal discussion we've had in a minute. Let's both parse it and see if there is anything further we should consider, and circle back tomorrow and finalize the plan & push. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan 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 ping ... - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan 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 IMO we should not send close_notify in the finalizer. It's the application's responsibility to send close_notify when it's done with the socket; we should not pretend that it was closed normally when it was not. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan 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
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Sat, 9 Apr 2022 14:59:13 GMT, Xue-Lei Andrew Fan wrote: > The existing behavior is trying to close the socket and send the > close_notify. As the socket closure has been done in the SocketImpl, I think > BaseSSLSocketImpl could rely on to the SocketImpl cleaner. So the left for > the cleaning is about the close_notify. @AlanBateman, just to clarify, send the close_notify first, then close the socket. >From the Socket's point of view, the encrypted close_notify message is just a >few bytes of application data sent over the Socket's OutputStream. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Sat, 9 Apr 2022 11:52:01 GMT, Alan Bateman wrote: > What is the existing behavior with the BaseSSLSocketImpl finalizer? Does it > just close the socket or does it to the close_notify before closing the > socket. If it does, and you want to keep that behavior, then you are probably > into significant refactoring to avoid a reference to the BaseSSLSocketImpl. The existing behavior is trying to close the socket and send the close_notify. As the socket closure has been done in the SocketImpl, I think BaseSSLSocketImpl could rely on to the SocketImpl cleaner. So the left for the cleaning is about the close_notify. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Fri, 8 Apr 2022 22:55:07 GMT, Bradford Wetmore wrote: > @AlanBateman, @dfuch, @bchristi-git, any great ideas here? As you have found, if an open Socket is unreferenced then a cleaner can close the underlying socket (and release the file descriptor). The Cleaner is setup by the SocketImpl implementation. What is the existing behavior with the BaseSSLSocketImpl finalizer? Does it just close the socket or does it to the close_notify before closing the socket. If it does, and you want to keep that behavior, then you are probably into significant refactoring to avoid a reference to the BaseSSLSocketImpl. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Fri, 8 Apr 2022 06:52:57 GMT, Xue-Lei Andrew Fan wrote: > The Socket implementation will take care of the file description/native > memory release, I think. I've walked through the network code and understand it now. The Socket creation code calls into sun.nio.ch.NioSocketImpl.create():451, which then allocates the FileDescriptor fd and assigns it to the Socket, then registers a closer for that FileDescriptor which will be triggered by the Socket GC. When the Socket is reclaimed, the FileDescriptor is released, but not by referencing the Socket itself. > It is expected to send the close_notify at the TLS layer. But the current > code using finalizer, which is not reliable. The underlying socket may have > been closed when the SSLSocket finalizing action is triggered. Generally, > application should call close() method explicitly, otherwise the finalizer is > not expect to work reliable. I was wondering it may be safe to remove the > finalizer. Yeah, I'm just not sure yet. > I agree that adding a best effort cleanup may be better. I was wondering to > check if it is possible to clean the socket in the socket creation factories > so that the object reference issues could be resolved. But as socket is a > kind of resource, application layer may make the clean up as well. > I'm still looking for a solution to clean up resource by using a method of > 'this'. Please advice if anyone has experiences. @AlanBateman, @dfuch, any great ideas here? - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan 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
> Thanks for the explanation: this is my first exposure to the
> `java.lang.ref.Cleaner` API, so am getting up to speed. Sorry if these are
> dumb comments/questions.
>
> I see now what was being talked about in your other PR: #8136 and to not use
> a reference to `this` which would keep it from being GC'd. I also see how
> you're keeping a cleaner object that has outside ("static") references to the
> actual things that need to be released, but don't we need to do the similar
> cleaning for the underlying Socket somehow? What do Sockets do to make sure
> the underlying file descriptors/native memory are properly released?
>
The Socket implementation will take care of the file description/native memory
release, I think.
> That said, we still need to send the close_notify at the TLS layer, right?
> Simply removing the finalize() method is just going to silently shutdown the
> connection, and then the Socket is going to do whatever it does for
> finalization/Cleaning.
It is expected to send the close_notify at the TLS layer. But the current code
using finalizer, which is not reliable. The underlying socket may have been
closed when the SSLSocket finalizing action is triggered. Generally,
application should call close() method explicitly, otherwise the finalizer is
not expect to work reliable. I was wondering it may be safe to remove the
finalizer.
I agree that adding a best effort cleanup may be better. I was wondering to
check if it is possible to clean the socket in the socket creation factories so
that the object reference issues could be resolved. But as socket is a kind of
resource, application layer may make the clean up as well.
Socket s = ...
cleaner.register(this, s::close);
I'm still looking for a solution to clean up resource by using a method of
'this'. Please advice if anyone has experiences.
-
PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan 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
Thanks for the explanation: this is my first exposure to the
`java.lang.ref.Cleaner` API, so am getting up to speed. Sorry if these are
dumb comments/questions.
I see now what was being talked about in your other PR:
https://github.com/openjdk/jdk/pull/8136 and to not use a reference to `this`
which would keep it from being GC'd. I also see how you're keeping a cleaner
object that has outside ("static") references to the actual things that need to
be released, but don't we need to do the similar cleaning for the underlying
Socket somehow? What do Sockets do to make sure the underlying file
descriptors/native memory are properly released?
That said, we still need to send the close_notify at the TLS layer, right?
Simply removing the finalize() method is just going to silently shutdown the
connection, and then the Socket is going to do whatever it does for
finalization/Cleaning.
-
PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8065/files - new: https://git.openjdk.java.net/jdk/pull/8065/files/38700012..ccfc42da Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8065=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8065=00-01 Stats: 30 lines in 1 file changed: 0 ins; 30 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8065/head:pull/8065 PR: https://git.openjdk.java.net/jdk/pull/8065
