Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AMQ-9563] JDK 21 support: Remove usage of java.security.AccessController from ActiveMQConnectionFactory #1296

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mattrpav
Copy link
Contributor

@mattrpav mattrpav commented Sep 3, 2024

  • java.security.AccessController is deprecated for removal
  • Keep try-catch-log around property lookup in the event a security exception is thrown on older JDKs.
  • Document JDK 21 refactor TODO with a comment for future refactor for JAAS API change

@mattrpav mattrpav self-assigned this Sep 3, 2024
@mattrpav mattrpav force-pushed the AMQ-9563 branch 3 times, most recently from 5dcd7ff to bd31b55 Compare September 20, 2024 14:44
@mattrpav mattrpav changed the title WIP: [AMQ-9563] Remove usage of java.security.AccessController where possi… [AMQ-9563] Remove usage of java.security.AccessController where possi… Sep 20, 2024
@mattrpav mattrpav changed the title [AMQ-9563] Remove usage of java.security.AccessController where possi… [AMQ-9563] JDK 21 support: Remove usage of java.security.AccessController from ActiveMQConnectionFactory Sep 20, 2024
Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this PR doesn't change this but it's weird to me that the code just logs a debug if an exception is thrown if properties are unreadable. I would think that exception would be a bigger deal and should be thrown to error out or at least logged as a warn or error. That would be a change outside of the scope of this PR of course, I just wanted to note that it's probably handled poorly.

@mattrpav
Copy link
Contributor Author

mattrpav commented Sep 23, 2024

@cshannon Good point. I had opted to leave the logging level alone. If there were an exception, it would be a SecurityException (in future JDKs this will cease to be a code path), NPE, or some fatal RuntimeException like OOM, interrupted, etc.

I'd be in favor of changing the level here to a WARN or ERROR. Perhaps best as a follow-on for 7.x to not break any existing log capture?

@cshannon
Copy link
Contributor

I'd be in favor of changing the level here to a WARN or ERROR. Perhaps best as a follow-on for 7.x to not break any existing log capture?

We should probably just change it in 6.2.0 or 6.3.0 etc, because with removing the security check it's pretty unlikely to fail. And without the security check if it does fail there's probably something much more serious going on if you can't check a system roperty, so I would think logging it as a WARN level would make sense and maybe in 7.x actually throw an exception.

@mattrpav
Copy link
Contributor Author

mattrpav commented Sep 23, 2024

I'd be in favor of changing the level here to a WARN or ERROR. Perhaps best as a follow-on for 7.x to not break any existing log capture?

We should probably just change it in 6.2.0 or 6.3.0 etc, because with removing the security check it's pretty unlikely to fail. And without the security check if it does fail there's probably something much more serious going on if you can't check a system roperty, so I would think logging it as a WARN level would make sense and maybe in 7.x actually throw an exception.

A quick check confirmed that the logger only exists for these two log messages, it may be better to get rid of the logger completely in 7.x and throw an exception?

  1. Merge this as-is in 6.2.0 (Leave log level alone)
  2. Remove logger in 7.x and throw an exception

ref: AMQ-9586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants