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: 8284105: Update security libraries to use sealed classes [v2]
On Fri, 8 Apr 2022 20:07:32 GMT, Sean Mullan wrote: >> Please review these changes to update the security libraries to use sealed >> classes. See JEP 409 for more details on sealed classes. >> >> No CSR is required as all the changes are to internal classes. A few classes >> that did not have subclasses were simply marked final instead of sealed. > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Make some classes package-private instead of sealed. Only one comment. Others look fine. src/java.security.jgss/share/classes/sun/security/krb5/KrbTgsRep.java line 43: > 41: * Kerberos client. > 42: */ > 43: final class KrbTgsRep extends KrbKdcRep { Make `KrbAsRep` final also to be symmetric. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8165
Re: RFR: 8284105: Update security libraries to use sealed classes [v2]
> Please review these changes to update the security libraries to use sealed > classes. See JEP 409 for more details on sealed classes. > > No CSR is required as all the changes are to internal classes. A few classes > that did not have subclasses were simply marked final instead of sealed. Sean Mullan has updated the pull request incrementally with one additional commit since the last revision: Make some classes package-private instead of sealed. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8165/files - new: https://git.openjdk.java.net/jdk/pull/8165/files/52aa0ad6..c27b1ba0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8165=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8165=00-01 Stats: 93 lines in 8 files changed: 42 ins; 38 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/8165.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8165/head:pull/8165 PR: https://git.openjdk.java.net/jdk/pull/8165
Re: RFR: 8284105: Update security libraries to use sealed classes [v2]
On Fri, 8 Apr 2022 16:11:27 GMT, Weijun Wang wrote: >> Sean Mullan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Make some classes package-private instead of sealed. > > src/java.base/share/classes/sun/security/rsa/RSASignature.java line 50: > >> 48: * @author Andreas Sterbenz >> 49: */ >> 50: public abstract class RSASignature extends SignatureSpi { > > We can probably move the `RSASignature.encodeSignature` method to `RSAUtil` > and this class can be package private. Ok. I'll also move `RSASignature.decodeSignature` to `RSAUtil` to maintain symmetry even though it isn't called outside the package. > src/java.base/share/classes/sun/security/util/math/intpoly/IntegerPolynomial.java > line 64: > >> 62: */ >> 63: >> 64: public abstract class IntegerPolynomial implements IntegerFieldModuloP { > > Although we only have several implementations, I think this class is meant to > be freely extendable for whatever new modulus. We can always add new ones to the permits clause later. - PR: https://git.openjdk.java.net/jdk/pull/8165
Re: RFR: 8284105: Update security libraries to use sealed classes
On Fri, 8 Apr 2022 16:17:19 GMT, Weijun Wang wrote: > It looks `KrbTgsRep.java`, `Krb5ProxyCredential.java`, `Builder.java`, > `Vertex.java`, `Validator.java`, and `RSAKeyPairGenerator.java` can all be > made package private. Good point, although I would prefer to leave `Validator` as public for now until we are more sure of the compatibility risk as there have been external dependencies on it. It may be useful to apply the `sealed` modifier to package-private classes, but for this RFE that is not the goal, so I will remove the `sealed` modifier from these classes (if applicable) when I make them package-private. - PR: https://git.openjdk.java.net/jdk/pull/8165
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Fri, 8 Apr 2022 13:50:25 GMT, Weijun Wang wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update copyright year > > @bchristi-git showed me this test. You might try it on other objects: > > import org.ietf.jgss.GSSManager; > import org.ietf.jgss.GSSName; > > import java.util.WeakHashMap; > > public class A1 { > private static WeakHashMap whm = new WeakHashMap(); > public static void main(String[] args) throws Exception { > System.setProperty("sun.security.nativegss.debug", "true"); > System.setProperty("sun.security.jgss.native", "true"); > GSSName e = GSSManager.getInstance().createName("u1", > GSSName.NT_USER_NAME); > whm.put(e, null); > e = null; > for (int i = 0; i < 10; i++) { > System.gc(); > Thread.sleep(100); > } > if (whm.size() > 0) { > throw new RuntimeException("GSSName still reachable"); > } > } > } > > The test ensures the objects are GCed and there's no memory leak. You need to > inspect the debug output to make sure native resources are released. The > `NativeGSSContext` code still needs to be fixed. Thanks, @wangweij . Wherever practical, it would be good to include tests to confirm correct conversions from finalizer to Cleaner -- bugs can be subtle, and hard to spot. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284105: Update security libraries to use sealed classes
On Fri, 8 Apr 2022 13:40:37 GMT, Sean Mullan wrote: > Please review these changes to update the security libraries to use sealed > classes. See JEP 409 for more details on sealed classes. > > No CSR is required as all the changes are to internal classes. A few classes > that did not have subclasses were simply marked final instead of sealed. It looks safe to me as if compiling and test passed. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8165
Re: RFR: 8284105: Update security libraries to use sealed classes
On Fri, 8 Apr 2022 13:40:37 GMT, Sean Mullan wrote: > Please review these changes to update the security libraries to use sealed > classes. See JEP 409 for more details on sealed classes. > > No CSR is required as all the changes are to internal classes. A few classes > that did not have subclasses were simply marked final instead of sealed. It looks `KrbTgsRep.java`, `Krb5ProxyCredential.java`, `Builder.java`, `Vertex.java`, `Validator.java`, and `RSAKeyPairGenerator.java` can all be made package private. src/java.base/share/classes/sun/security/rsa/RSASignature.java line 50: > 48: * @author Andreas Sterbenz > 49: */ > 50: public abstract class RSASignature extends SignatureSpi { We can probably move the `RSASignature.encodeSignature` method to `RSAUtil` and this class can be package private. src/java.base/share/classes/sun/security/util/math/intpoly/IntegerPolynomial.java line 64: > 62: */ > 63: > 64: public abstract class IntegerPolynomial implements IntegerFieldModuloP { Although we only have several implementations, I think this class is meant to be freely extendable for whatever new modulus. - PR: https://git.openjdk.java.net/jdk/pull/8165
Re: A possible JEP to replace SecurityManager after JEP 411
On Fri, Apr 8, 2022 at 10:14 AM Sean Mullan wrote: > > Ok, thanks for some clarification on the proposal. > > How many applications currently depend on the SM for this type of usage? > What other alternate models have you considered? There are some number of customers and users within our user base who rely on SM for certain security certifications or requirements. The alternative would be to attempt to reframe these certifications or requirements on a case by case basis to exclude SM, which might or might not be less work than preserving it. > In general, I think authorization is best done at a higher layer within > the application and not via low-level SM callouts. Authorize the subject > first and if not acceptable, prevent the operation or API from being > called in the first place. Once the operation is in motion, you have > already taken a greater risk that something might go wrong. The low level authorization checks would be in addition to the high level checks that we already perform. But I understand your position. > > I hope this clarifies things. Like I said, "no" is an acceptable > > answer for us but I would be remiss if I didn't ensure that the "no" > > was based on an accurate understanding of what we are proposing, so > > hopefully this helps. > > It does help, but not enough to change my previous stance. OK, thanks for the feedback. -- - DML • he/him
Re: RFR: 8284105: Update security libraries to use sealed classes
On Fri, 8 Apr 2022 13:40:37 GMT, Sean Mullan wrote: > Please review these changes to update the security libraries to use sealed > classes. See JEP 409 for more details on sealed classes. > > No CSR is required as all the changes are to internal classes. A few classes > that did not have subclasses were simply marked final instead of sealed. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8165
Integrated: 8283698: Refactor Locale constructors used in src/test
On Wed, 6 Apr 2022 17:45:13 GMT, Naoto Sato wrote: > This is a follow-on task after deprecating the Locale constructors > (https://bugs.openjdk.java.net/browse/JDK-8282819). Most of the changes are > simple replacements to Locale constructors with `Locale.of()` or Locale > constants, such as `Locale.US`. This pull request has now been integrated. Changeset: d6b4693c Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/d6b4693c0527385f8999089b3f8b2120548efecb Stats: 750 lines in 182 files changed: 3 ins; 3 del; 744 mod 8283698: Refactor Locale constructors used in src/test Reviewed-by: iris, joehw - PR: https://git.openjdk.java.net/jdk/pull/8130
Re: A possible JEP to replace SecurityManager after JEP 411
Ok, thanks for some clarification on the proposal. How many applications currently depend on the SM for this type of usage? What other alternate models have you considered? In general, I think authorization is best done at a higher layer within the application and not via low-level SM callouts. Authorize the subject first and if not acceptable, prevent the operation or API from being called in the first place. Once the operation is in motion, you have already taken a greater risk that something might go wrong. > I hope this clarifies things. Like I said, "no" is an acceptable > answer for us but I would be remiss if I didn't ensure that the "no" > was based on an accurate understanding of what we are proposing, so > hopefully this helps. It does help, but not enough to change my previous stance. --Sean On 4/8/22 9:03 AM, David Lloyd wrote: Instead the API would exist to give containers and applications an extra layer of authorization which does not exist today. Hypothetically speaking, if even one authorization check is retained, then that is more than would exist if the API were removed. There would be no expectation that usage of this API conveys any kind of end to end security, and this would be explicitly conveyed in the API documentation.
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v3]
On Thu, 7 Apr 2022 19:21:31 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. 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 one > additional commit since the last revision: > > Update copyright year @bchristi-git showed me this test. You might try it on other objects: import org.ietf.jgss.GSSManager; import org.ietf.jgss.GSSName; import java.util.WeakHashMap; public class A1 { private static WeakHashMap whm = new WeakHashMap(); public static void main(String[] args) throws Exception { System.setProperty("sun.security.nativegss.debug", "true"); System.setProperty("sun.security.jgss.native", "true"); GSSName e = GSSManager.getInstance().createName("u1", GSSName.NT_USER_NAME); whm.put(e, null); e = null; for (int i = 0; i < 10; i++) { System.gc(); Thread.sleep(100); } if (whm.size() > 0) { throw new RuntimeException("GSSElementName still reachable"); } } } - PR: https://git.openjdk.java.net/jdk/pull/8136
RFR: 8284105: Update security libraries to use sealed classes
Please review these changes to update the security libraries to use sealed classes. See JEP 409 for more details on sealed classes. No CSR is required as all the changes are to internal classes. A few classes that did not have subclasses were simply marked final instead of sealed. - Commit messages: - Update security libraries to use sealed classes. Changes: https://git.openjdk.java.net/jdk/pull/8165/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8165=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284105 Stats: 50 lines in 20 files changed: 8 ins; 0 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/8165.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8165/head:pull/8165 PR: https://git.openjdk.java.net/jdk/pull/8165
Re: A possible JEP to replace SecurityManager after JEP 411
Small correction: On Fri, Apr 8, 2022 at 8:03 AM David Lloyd wrote: > Instead the API would exist to give containers and applications an > extra layer of authorization which does not exist today. Of course I meant "which would not exist after JEP 411". -- - DML • he/him
Re: A possible JEP to replace SecurityManager after JEP 411
Hi Sean, thanks for replying. I'll reply inline. On Thu, Apr 7, 2022 at 2:20 PM Sean Mullan wrote: > > With this proposal, as I understand it, the JDK would still be > responsible for maintaining and preserving essentially all of the > existing calls to the Security Manager (SM). I think we'd actually want to evaluate each case to decide whether to retain the permission check. But I didn't want to preemptively eliminate any check, especially considering that the `Permission` classes were not included in JEP 411. > All new code and APIs would > still need to be evaluated and determined if permission checks were > needed as well as making appropriate specification changes to note the > behavior when an SM is enabled (throwing a SecurityException, etc). Correct. > Any missing checks would need to be treated as security issues. This I disagree with: we are not proposing retention of this API for sandboxing purposes. In fact the only reason to keep the name `SecurityManager` is for compatibility purposes. Instead the API would exist to give containers and applications an extra layer of authorization which does not exist today. Hypothetically speaking, if even one authorization check is retained, then that is more than would exist if the API were removed. There would be no expectation that usage of this API conveys any kind of end to end security, and this would be explicitly conveyed in the API documentation. > And we would > still need to test the code and APIs to ensure that it worked properly > and complied with the API specification. This would likely mean > implementing and maintaining an internal SM implementation in OpenJDK. Only within the test suite, and this would be expected to be trivial implementations only: always allow vs always throw. > The proposal also includes retaining calls to doPrivileged (but later > potentially replacing them with some other mechanism TBD). No, this is incorrect; the proposal includes retaining the *methods*, not the calls; this is explicitly left off of the proposal. > The JDK source code includes over 1000 calls to doPrivileged. Each of these > need > to be carefully reviewed to ensure that they do not contain security > issues and any new code needs to be evaluated to see if new calls to > doPrivileged are necessary. Under this proposal, it would be perfectly acceptable to eliminate these calls, and put the onus on the implementation to decide the circumstances of the invocation. The JDK would be under no obligation to provide any level of actual security - it would only be obligated to use the installed SecurityManager, if any, for authorization checks at some subset of the locations at which an authorization check currently takes place. > Retaining doPrivileged (or something similar) means that there can be > domains of code with different permissions running within the VM, which > retains much of the complexity of the current SM model. The only reason to retain these methods is the massive amount of existing code which is doing something along the lines of: if (System.getSecurityManager() != null) { doPrivileged(something); } else { something.run(); } It has nothing to do with what is in the JDK, which is why this item was marked "optional" in the initial proposal. > In this proposal, how privileges are established or propagated is > implementation-specific. But how could applications or libraries depend > on the APIs and still have some confidence that the code is behaving > consistently and securely? It is already an error to assume that code is behaving consistently and securely on the sole basis of the presence of a SecurityManager, and this would not change; the major difference is that the API would be able to explicitly document this fact. The value that the user gains from using a SecurityManager versus not using one would be that JDK operations would be authorization checked. > Today, the cost of buying into the SM model is high for libraries and > applications. Not many third party libraries support the SM and have > modified their code to perform permission checks and call doPrivileged > in the right places. If there were pluggable SMs each behaving > differently, there would likely be less incentive. The point of retaining these APIs isn't necessarily to increase usage; it is to provide some continuity for things which already expect some level of authorization from the JDK. We think that it is the case that this would add some value for our customers and users which offsets the value lost by the removal of SM. The question is, is the value enough to make our efforts to make this happen worthwhile? This is not only a question of the cost of developing the JEP and implementation effort, but also a question of how difficult it will be to gain enough acceptance of the idea in the upstream community that the change could potentially be integrated. > Although it sounds beneficial to be able to delegate the SM > implementation
Re: A possible JEP to replace SecurityManager after JEP 411
Thanks for trying David. :) Regards, Peter. On 8/04/2022 7:15 pm, Andrew Dinn wrote: I'm 100% in agreement with Sean. This proposal leaves the OpenJDK team with just as much -- or possibly more -- code to maintain, test and design around while making the behaviour under the retained API less determinate, less reliable as a security enforcement mechanism and, in consequence, even less likely to be used than it is already. regards, Andrew Dinn --- Red Hat Distinguished Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill On 07/04/2022 20:19, Sean Mullan wrote: Hi David, Thanks for the feedback and spending some time on this proposal. Some specific comments below. On 4/5/22 9:52 AM, David Lloyd wrote: Here at Red Hat there have been serious discussions about the impacts of security manager removal on our users, and whether there is an actual value impact, and if so, whether it can be mitigated or reversed somehow. We are interested in exploring whether we can come up with a way in which vendors and projects that wish to continue using SecurityManager (or something like it) would be able to do so, while still removing the majority of the ongoing maintenance burden from the OpenJDK project. Before we make a decision on whether or not we think there is sufficient justification for working up a formal JEP, we have decided that the best first step would be to socialize the idea in a more general form so that we can know whether the upstream OpenJDK team would even be amenable *at all* to the solution (or something like it), particularly in light of the observation that previous threads about retaining SecurityManager in any form have been looked upon in a fairly negative light. The primary idea behind this proposal is that, while all of the points in JEP 411 relating to the lack of what most experts might refer to as "actual security" are certainly true, the SecurityManager mechanism itself does nevertheless have some inherent value. The challenge, then, is to strike a balance between the value provided by retaining some semblance of the mechanism versus the costs inherent in retaining it; we would want as much of the former as possible, for as little of the latter as possible. With this proposal, as I understand it, the JDK would still be responsible for maintaining and preserving essentially all of the existing calls to the Security Manager (SM). All new code and APIs would still need to be evaluated and determined if permission checks were needed as well as making appropriate specification changes to note the behavior when an SM is enabled (throwing a SecurityException, etc). Any missing checks would need to be treated as security issues. And we would still need to test the code and APIs to ensure that it worked properly and complied with the API specification. This would likely mean implementing and maintaining an internal SM implementation in OpenJDK. The proposal also includes retaining calls to doPrivileged (but later potentially replacing them with some other mechanism TBD). The JDK source code includes over 1000 calls to doPrivileged. Each of these need to be carefully reviewed to ensure that they do not contain security issues and any new code needs to be evaluated to see if new calls to doPrivileged are necessary. Retaining doPrivileged (or something similar) means that there can be domains of code with different permissions running within the VM, which retains much of the complexity of the current SM model. In this proposal, how privileges are established or propagated is implementation-specific. But how could applications or libraries depend on the APIs and still have some confidence that the code is behaving consistently and securely? Today, the cost of buying into the SM model is high for libraries and applications. Not many third party libraries support the SM and have modified their code to perform permission checks and call doPrivileged in the right places. If there were pluggable SMs each behaving differently, there would likely be less incentive. Although it sounds beneficial to be able to delegate the SM implementation to a 3rd-party, in reality, I think very few people would take the time to implement it securely, and instead would mostly leverage its power to do things that aren't at all security related. Sure, removing the default SM and Policy implementation reduces the complexity a little, but there would still be a fairly significant maintenance overhead and an additional drawback that it would make it more difficult for applications and libraries to depend on any type of consistent behavior. --Sean So, here's the idea. It is assumed (for the sake of common understanding) that as things stand, all of the classes and members marked as "deprecated for removal" as a part of JEP 411 are intended to be completely
Re: A possible JEP to replace SecurityManager after JEP 411
I'm 100% in agreement with Sean. This proposal leaves the OpenJDK team with just as much -- or possibly more -- code to maintain, test and design around while making the behaviour under the retained API less determinate, less reliable as a security enforcement mechanism and, in consequence, even less likely to be used than it is already. regards, Andrew Dinn --- Red Hat Distinguished Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill On 07/04/2022 20:19, Sean Mullan wrote: Hi David, Thanks for the feedback and spending some time on this proposal. Some specific comments below. On 4/5/22 9:52 AM, David Lloyd wrote: Here at Red Hat there have been serious discussions about the impacts of security manager removal on our users, and whether there is an actual value impact, and if so, whether it can be mitigated or reversed somehow. We are interested in exploring whether we can come up with a way in which vendors and projects that wish to continue using SecurityManager (or something like it) would be able to do so, while still removing the majority of the ongoing maintenance burden from the OpenJDK project. Before we make a decision on whether or not we think there is sufficient justification for working up a formal JEP, we have decided that the best first step would be to socialize the idea in a more general form so that we can know whether the upstream OpenJDK team would even be amenable *at all* to the solution (or something like it), particularly in light of the observation that previous threads about retaining SecurityManager in any form have been looked upon in a fairly negative light. The primary idea behind this proposal is that, while all of the points in JEP 411 relating to the lack of what most experts might refer to as "actual security" are certainly true, the SecurityManager mechanism itself does nevertheless have some inherent value. The challenge, then, is to strike a balance between the value provided by retaining some semblance of the mechanism versus the costs inherent in retaining it; we would want as much of the former as possible, for as little of the latter as possible. With this proposal, as I understand it, the JDK would still be responsible for maintaining and preserving essentially all of the existing calls to the Security Manager (SM). All new code and APIs would still need to be evaluated and determined if permission checks were needed as well as making appropriate specification changes to note the behavior when an SM is enabled (throwing a SecurityException, etc). Any missing checks would need to be treated as security issues. And we would still need to test the code and APIs to ensure that it worked properly and complied with the API specification. This would likely mean implementing and maintaining an internal SM implementation in OpenJDK. The proposal also includes retaining calls to doPrivileged (but later potentially replacing them with some other mechanism TBD). The JDK source code includes over 1000 calls to doPrivileged. Each of these need to be carefully reviewed to ensure that they do not contain security issues and any new code needs to be evaluated to see if new calls to doPrivileged are necessary. Retaining doPrivileged (or something similar) means that there can be domains of code with different permissions running within the VM, which retains much of the complexity of the current SM model. In this proposal, how privileges are established or propagated is implementation-specific. But how could applications or libraries depend on the APIs and still have some confidence that the code is behaving consistently and securely? Today, the cost of buying into the SM model is high for libraries and applications. Not many third party libraries support the SM and have modified their code to perform permission checks and call doPrivileged in the right places. If there were pluggable SMs each behaving differently, there would likely be less incentive. Although it sounds beneficial to be able to delegate the SM implementation to a 3rd-party, in reality, I think very few people would take the time to implement it securely, and instead would mostly leverage its power to do things that aren't at all security related. Sure, removing the default SM and Policy implementation reduces the complexity a little, but there would still be a fairly significant maintenance overhead and an additional drawback that it would make it more difficult for applications and libraries to depend on any type of consistent behavior. --Sean So, here's the idea. It is assumed (for the sake of common understanding) that as things stand, all of the classes and members marked as "deprecated for removal" as a part of JEP 411 are intended to be completely removed without replacement at the end of the term of deprecation. The proposals here are
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