8212136: Remove finalizer implementation in SSLSocketImpl #8065

XueleiFan · 2022-03-31T20:15:35Z

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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 reviews required, with at least 1 reviewer)
Change requires a CSR request to be approved

Issues

JDK-8212136: Remove finalizer implementation in SSLSocketImpl
JDK-8286064: Remove finalizer implementation in SSLSocketImpl (CSR)

Reviewers

Bradford Wetmore (@bradfordwetmore - Reviewer) ⚠️ Review applies to ccfc42da

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8065/head:pull/8065
$ git checkout pull/8065

Update a local copy of the PR:
$ git checkout pull/8065
$ git pull https://git.openjdk.java.net/jdk pull/8065/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8065

View PR using the GUI difftool:
$ git pr show -t 8065

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8065.diff

bridgekeeper · 2022-03-31T20:15:55Z

👋 Welcome back xuelei! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-03-31T20:18:24Z

@XueleiFan The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-03-31T20:22:01Z

Webrevs

XueleiFan · 2022-04-05T19:21:05Z

ping ...

XueleiFan · 2022-04-07T19:54:31Z

Need an update, close the review for now.

XueleiFan · 2022-04-07T20:11:25Z

The socket close() call in the finalize() method may be blocked for the SSL implementation, which is not good for garbage collection. It should be safe by just removing the finalize() method.

bradfordwetmore · 2022-04-07T22:49:24Z

The socket close() call in the finalize() method may be blocked for the SSL implementation, which is not good for garbage collection. It should be safe by just removing the finalize() method.

Can you provide more detail on why simply removing the finalize() method is ok? I expected something more like your first attempt (java.lang.ref.Cleaner) that would properly close/send the close_notify message. Obviously there is no need to wait for the reply from the peer. Thanks!

XueleiFan · 2022-04-07T23:09:03Z

The socket close() call in the finalize() method may be blocked for the SSL implementation, which is not good for garbage collection. It should be safe by just removing the finalize() method.
> Can you provide more detail? I expected something more like your first attempt (java.lang.ref.Cleaner`) that would properly close/send the close_notify message. Thanks!

The 1st use of Cleaner refer to 'this' object, as result in that 'this' object cannot be phantom reachable and thus the cleaner cannot be triggered. As the close() is a method of 'this' object, the calling to close() in a cleaner will hold a reference to 'this' object, and make the cleaner failed (and memory leak).

bradfordwetmore · 2022-04-08T01:45:52Z

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?

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.

XueleiFan · 2022-04-08T06:52:57Z

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.

bradfordwetmore · 2022-04-08T22:55:07Z

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, @bchristi-git, any great ideas here?

AlanBateman · 2022-04-09T11:52:01Z

@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.

XueleiFan · 2022-04-09T14:59:13Z

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.

bradfordwetmore · 2022-04-09T17:01:44Z

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.

XueleiFan · 2022-04-14T06:33:58Z

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.

djelinski · 2022-04-14T15:37:05Z

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.

XueleiFan · 2022-04-27T06:55:42Z

ping ...

bradfordwetmore · 2022-04-29T01:46:55Z

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.

bradfordwetmore

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.

@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.

openjdk · 2022-04-30T03:39:08Z

@XueleiFan This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8212136: Remove finalizer implementation in SSLSocketImpl

Reviewed-by: wetmore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 413 new commits pushed to the master branch:

1bb4de2: 8285956: (fs) Excessive default poll interval in PollingWatchService
3092b56: 8284813: x86 Code cleanup related to move instructions.
d43ae72: 8286105: SourceRevision.gmk should respect GIT variable
7d54508: 8283606: Tests may fail with zh locale on MacOS
158d871: 8285693: Create an automated test for JDK-4702199
bb022b2: 8285470: Improve handling of @inheritdoc
09e6ee9: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms
4d30a1e: 8284220: TypeMirror#toString omits enclosing class names after JDK-8281238
28e6d80: 8285869: Selective cleanup in doclint Checker class
00e6c63: 8277090: jsr166 refresh for jdk19
... and 403 more: https://git.openjdk.java.net/jdk/compare/ad83ec7e281cb3ab7c42e71fab47ea21b93079ea...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

bradfordwetmore · 2022-04-30T03:46:23Z

src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java

@@ -262,37 +262,6 @@ public boolean isOutputShutdown() {
        }
    }

-    /**


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.

It may be not common to comment on non-existing method. Maybe, the class description could be a place to add this note.

XueleiFan · 2022-05-02T04:56:48Z

@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.

bradfordwetmore · 2022-05-02T16:46:17Z

src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java

@@ -41,6 +41,11 @@
 * Methods are defined final to ensure that they are not accidentally
 * overridden in SSLSocketImpl.
 *
+ * There used to be a finalize() implementation, but decided that was


Since you haven't pushed yet, maybe:

There used to be a finalize() implementation that sent close_notify's, but decided that was not needed. Not closing properly is more properly an error condition that should be avoided. Applications should close sockets and not rely on garbage collection. The underlying native resources are handled by the Socket Cleaner.

Thanks for the rewording. Updated.

Thanks for the rewording. Updated.

I made one more tweak that reads better.

Yes, it looks better. Updated. Thanks!

Looks good, thanks.

dfuch · 2022-05-03T13:03:33Z

Given that this proposed change will change the behavior for SSLSocket (& SSLServerSocket?) I would also suggest to file a CSR before integrating.

XueleiFan · 2022-05-03T15:18:10Z

/csr

openjdk · 2022-05-03T15:18:27Z

@XueleiFan an approved CSR request is already required for this pull request.

XueleiFan · 2022-05-03T15:33:45Z

CSR and release note requests are filed in JBS. May I have them reviewed?

CSR: https://bugs.openjdk.java.net/browse/JDK-8286064
Release Note: https://bugs.openjdk.java.net/browse/JDK-8286065

dfuch · 2022-05-03T16:16:08Z

/csr needed

openjdk · 2022-05-03T16:16:38Z

@dfuch an approved CSR request is already required for this pull request.

XueleiFan · 2022-05-03T23:10:51Z

Could someone in Oracle help to run the Mach5 testing for security and network components (or just tier1 and tier2), and let me know if this update causes any failures?

bradfordwetmore · 2022-05-04T00:47:49Z

Could someone in Oracle help to run the Mach5 testing for security and network components (or just tier1 and tier2), and let me know if this update causes any failures?

Builds underway.

bradfordwetmore · 2022-05-04T16:12:43Z

tier1/tier2 passed.

bradfordwetmore · 2022-05-04T19:21:50Z

linux-x64 Infra + JCK passed. Looks good.

seanjmullan · 2022-05-05T20:45:09Z

Even though this doesn't appear to be a big change, I would advise not integrating until next week as the Loom integration into 19 is planned for this weekend and it would be better to have as few disruptions as possible. Thanks!

XueleiFan · 2022-05-09T14:13:56Z

/integrate

openjdk · 2022-05-09T14:15:31Z

Going to push as commit 034f20f.
Since your change was applied there have been 457 commits pushed to the master branch:

36e4df9: 8285516: clearPassword should be called in a finally try block
b849efd: 8285923: [REDO] JDK-8285802 AArch64: Consistently handle offsets in MacroAssembler as 64-bit quantities
f143386: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources
64b05cc: 8286346: 3-parameter version of AllocateHeap should not ignore AllocFailType
4f5d73f: 8286294: ForkJoinPool.commonPool().close() spins
d4474b5: 8285794: AsyncGetCallTrace might acquire a lock via JavaThread::thread_from_jni_environment
39f4434: 8286312: Stop mixing signed and unsigned types in bit operations
b490a58: 8283899: Revert 8284190 after fix of 8281297
1ce72ea: 8281297: TestStressG1Humongous fails with guarantee(is_range_uncommitted)
cdd1b0d: 8284613: invalid use of @serial tag
... and 447 more: https://git.openjdk.java.net/jdk/compare/ad83ec7e281cb3ab7c42e71fab47ea21b93079ea...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-09T14:15:48Z

@XueleiFan Pushed as commit 034f20f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8212136: Remove BaseSSLSocketImpl finalizer method

3870001

openjdk bot added the rfr Pull request is ready for review label Mar 31, 2022

openjdk bot added the security security-dev@openjdk.org label Mar 31, 2022

XueleiFan closed this Apr 7, 2022

revise the update

9aff6fb

XueleiFan reopened this Apr 7, 2022

typo blank linke correction

ccfc42d

bradfordwetmore approved these changes Apr 30, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 30, 2022

bradfordwetmore approved these changes Apr 30, 2022

View reviewed changes

comment about remove finalize() method

c90e25a

bradfordwetmore reviewed May 2, 2022

View reviewed changes

Xuelei Fan added 2 commits May 2, 2022 10:44

update the note

6130f5e

More note update

8caf85a

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels May 3, 2022

openjdk bot removed the csr Pull request needs approved CSR before integration label May 4, 2022

XueleiFan changed the title ~~8212136: Remove BaseSSLSocketImpl finalizer method~~ 8212136: Remove finalizer implementation in SSLSocketImpl May 5, 2022

openjdk bot added the ready Pull request is ready to be integrated label May 5, 2022

openjdk bot added the integrated Pull request has been integrated label May 9, 2022

openjdk bot closed this May 9, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 9, 2022

XueleiFan deleted the JDK-8212136 branch May 9, 2022 14:17

@@ @@ -262,37 +262,6 @@ public boolean isOutputShutdown() { @@
                       }
                   }
-                  /**

8212136: Remove finalizer implementation in SSLSocketImpl #8065

8212136: Remove finalizer implementation in SSLSocketImpl #8065

Uh oh!

Conversation

XueleiFan commented Mar 31, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 31, 2022

Uh oh!

openjdk bot commented Mar 31, 2022

Uh oh!

mlbridge bot commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

XueleiFan commented Apr 5, 2022

Uh oh!

XueleiFan commented Apr 7, 2022

Uh oh!

XueleiFan commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradfordwetmore commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XueleiFan commented Apr 7, 2022

Uh oh!

bradfordwetmore commented Apr 8, 2022

Uh oh!

XueleiFan commented Apr 8, 2022

Uh oh!

bradfordwetmore commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanBateman commented Apr 9, 2022

Uh oh!

XueleiFan commented Apr 9, 2022

Uh oh!

bradfordwetmore commented Apr 9, 2022

Uh oh!

XueleiFan commented Apr 14, 2022

Uh oh!

djelinski commented Apr 14, 2022

Uh oh!

XueleiFan commented Apr 27, 2022

Uh oh!

bradfordwetmore commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradfordwetmore left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradfordwetmore Apr 30, 2022

Choose a reason for hiding this comment

Uh oh!

XueleiFan May 2, 2022

Choose a reason for hiding this comment

Uh oh!

XueleiFan commented May 2, 2022

Uh oh!

bradfordwetmore May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XueleiFan May 2, 2022

Choose a reason for hiding this comment

Uh oh!

bradfordwetmore May 2, 2022

Choose a reason for hiding this comment

Uh oh!

XueleiFan May 3, 2022

Choose a reason for hiding this comment

XueleiFan commented Mar 31, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 31, 2022 •

edited

Loading

XueleiFan commented Apr 7, 2022 •

edited

Loading

bradfordwetmore commented Apr 7, 2022 •

edited

Loading

bradfordwetmore commented Apr 8, 2022 •

edited

Loading

bradfordwetmore commented Apr 29, 2022 •

edited

Loading

bradfordwetmore left a comment •

edited

Loading

openjdk bot commented Apr 30, 2022 •

edited

Loading

bradfordwetmore May 2, 2022 •

edited

Loading

XueleiFan commented May 3, 2022 •

edited

Loading

bradfordwetmore commented May 4, 2022 •

edited

Loading