RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]

[Daniel Fuchs]

On Mon, 17 Jun 2024 20:27:05 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Hi,
>> We almost always check !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the RMIConnectionImpl contstructor)
>> 
>> This keeps the "modern" case first (except in that constructor, where it is only one line for each side of the if...).
>> 
>> This extra checking of SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the modern and old style cases distinct, based on other comments here.  It keeps it simple to read I hope, but yes it is a little more verbose than it could be.
>> 
>> I'm hoping you'll agree that as these checks will be removed anyway, probably with a release, we should delay more extensive cleanup until then.  (I've also  rolled back my noPermissionsACC removal to keep this change away from being a general cleanup.)
>> 
>> I had a bunch of additional Exception handling for e.g. PrivilegedActionException I think in here, and it wasn't very pretty.  But, it might look better when there are fewer nested ifs in these methods, and when we are properly in the post-SecurityManager world... 8-)
>> 
>> Whether it checks acc != null or acc == null is based on the existing code.  I think that makes this diff easier to read, but also could be reworked and be made more consistent after SM removal.
>
> Agree with Kevin. To minimize risk, especially since this is to fix a significant regression and we are in RDP1, we are really trying to preserve the existing code as much as possible, even though it could be improved.

It is also more error prone (which is why I suggested using a single well tested utility method to implement the temporary hackery rather than spreading it at different places in different forms). But I'm OK with the code in this form.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644303322