HSEARCH-5131 Remove usage and mentions of a security manager #4124
Conversation
| @@ -115,7 +115,7 @@ private static <T> Constructor<T> getConstructor(TypeToken<T> type) { | |||
| try { | |||
| return (Constructor<T>) type.getRawType().getConstructor(); | |||
| } | |||
| catch (NoSuchMethodException | SecurityException e) { | |||
There was a problem hiding this comment.
References to SecurityException do not need to be removed, because:
-
It's a runtime exception, so our code will compile even if the code inside the try block does not declare throwing that exception.
-
SecurityExceptionis not planned for removal at the moment:We will not deprecate some classes in the java.security package that are related to the Security Manager, for various reasons:
- SecurityException — A runtime exception thrown by Java APIs when a permission check fails. We may deprecate this API for removal at a later date, but for now the impact of doing so would be too high.
IMO we'd better keep those during the transition period where we still support JDKs that could possibly be using the security manager.
There was a problem hiding this comment.
mm, ok. My thoughts here were that since the security exception can only occur in these places when SM is involved (at least according to the javadocs 😃) and we are "removing SM" there's no point in keeping it.
| @SuppressForbiddenApis(reason = "It's unclear how we will handle this without the security manager;" | ||
| + " we'll see when the security manager actually gets removed from the JDK") | ||
| public ThreadFactory createThreadFactory(String prefix) { | ||
| @SuppressWarnings("removal") | ||
| SecurityManager s = System.getSecurityManager(); | ||
| @SuppressWarnings({ "removal", "deprecation" }) | ||
| ThreadGroup group = ( s != null ) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup(); |
There was a problem hiding this comment.
Should we have some ugly static reflection code that detects whether System.getSecurityManager exists, and depending on that does the right thing?
I have no idea of the impact of this change on existing apps... Don't get me wrong, I'm fine with this changing behavior, but if it does, we'll need to postpone the change to a major version of Search... But maybe you're planning on having one anyway for when ORM 7.0 is out?
There was a problem hiding this comment.
hmm so maybe let's do the reflection thing and keep the Security Exceptions in place and get this merged into the current main (7.2) and then for the next major we remove all that ?
There was a problem hiding this comment.
Works for me. Though at this point you may as well open another PR and keep this one for the next major :)
BTW, do we have a version of the JDK we could test the reflection hack against? I think everything still works on JDK 23, doesn't it? I'll do a JDK upgrade on Jenkins just in case.
There was a problem hiding this comment.
I'm looking at 23.ea.18-open, and it seems like it still has SM in place...
|
https://hibernate.atlassian.net/browse/HSEARCH-5131
I could only find the thread group and catching a few security exceptions related to the SM in Search.