-
Notifications
You must be signed in to change notification settings - Fork 242
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
HSEARCH-5131 Remove usage and mentions of a security manager #4124
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ | |
import java.util.concurrent.ThreadFactory; | ||
|
||
import org.hibernate.search.engine.environment.thread.spi.ThreadProvider; | ||
import org.hibernate.search.util.common.annotation.impl.SuppressForbiddenApis; | ||
|
||
public final class EmbeddedThreadProvider implements ThreadProvider { | ||
|
||
|
@@ -31,13 +30,8 @@ public String createThreadName(String prefix, int threadNumber) { | |
} | ||
|
||
@Override | ||
@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(); | ||
Comment on lines
-34
to
-40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have some ugly static reflection code that detects whether 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm so maybe let's do the reflection thing and keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm looking at 23.ea.18-open, and it seems like it still has SM in place... |
||
ThreadGroup group = Thread.currentThread().getThreadGroup(); | ||
String namePrefix = createFullThreadNamePrefix( prefix ); | ||
return new SimpleThreadFactory( group, namePrefix ); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
SecurityException
is not planned for removal at the moment:-- https://openjdk.org/jeps/411#subject-doas
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.