uschindler commented on code in PR #15864:
URL: https://github.com/apache/lucene/pull/15864#discussion_r2976139672
##########
lucene/core/src/java/org/apache/lucene/internal/tests/TestSecrets.java:
##########
@@ -31,26 +31,20 @@
* initialized once on startup.
*/
public final class TestSecrets {
- static {
- Consumer<Class<?>> ensureInitialized =
- clazz -> {
- try {
- // A no-op forName here has a side-effect of ensuring the class is
loaded and
- // initialized.
- // This only happens once. We could just leverage the JLS and
invoke a static
- // method (or a constructor) on the target class but the method
below seems simpler.
- // TODO: In Java 15 there's
MethodHandles.lookup().ensureInitialized(clazz)
- Class.forName(clazz.getName());
- } catch (ClassNotFoundException e) {
- throw new RuntimeException(e);
- }
- };
-
- ensureInitialized.accept(ConcurrentMergeScheduler.class);
- ensureInitialized.accept(SegmentReader.class);
- ensureInitialized.accept(IndexWriter.class);
- ensureInitialized.accept(FilterIndexInput.class);
- }
+
+ private static final Consumer<Class<?>> ensureInitialized =
Review Comment:
we can make a real private, static method here, the Lambda was only a
workaround to not define a method just for the static initializer. As we need
to call it lazily we can also have a method.
##########
lucene/core/src/java/org/apache/lucene/internal/tests/TestSecrets.java:
##########
@@ -71,60 +65,80 @@ private TestSecrets() {}
/** Return the accessor to internal secrets for an {@link IndexReader}. */
public static IndexPackageAccess getIndexPackageAccess() {
- ensureCaller();
+ ensureCallerForGetter();
+ if (indexWriterAccess == null) {
+ ensureInitialized.accept(IndexWriter.class);
+ }
return Objects.requireNonNull(indexPackageAccess);
}
/** Return the accessor to internal secrets for an {@link
ConcurrentMergeScheduler}. */
public static ConcurrentMergeSchedulerAccess
getConcurrentMergeSchedulerAccess() {
- ensureCaller();
+ ensureCallerForGetter();
+ if (cmsAccess == null) {
+ ensureInitialized.accept(ConcurrentMergeScheduler.class);
+ }
return Objects.requireNonNull(cmsAccess);
}
/** Return the accessor to internal secrets for an {@link SegmentReader}. */
public static SegmentReaderAccess getSegmentReaderAccess() {
- ensureCaller();
+ ensureCallerForGetter();
+ if (segmentReaderAccess == null) {
+ ensureInitialized.accept(SegmentReader.class);
+ }
return Objects.requireNonNull(segmentReaderAccess);
}
/** Return the accessor to internal secrets for an {@link IndexWriter}. */
public static IndexWriterAccess getIndexWriterAccess() {
- ensureCaller();
+ ensureCallerForGetter();
+ if (indexWriterAccess == null) {
+ ensureInitialized.accept(IndexWriter.class);
+ }
return Objects.requireNonNull(indexWriterAccess);
}
/** Return the accessor to internal secrets for an {@link FilterIndexInput}.
*/
public static FilterIndexInputAccess getFilterInputIndexAccess() {
- ensureCaller();
+ ensureCallerForGetter();
+ if (filterIndexInputAccess == null) {
+ ensureInitialized.accept(FilterIndexInput.class);
+ }
return Objects.requireNonNull(filterIndexInputAccess);
}
/** For internal initialization only. */
public static void setIndexWriterAccess(IndexWriterAccess indexWriterAccess)
{
+ ensureCallerForSetter(IndexWriter.class);
Review Comment:
That's nice that we check this now, but this change is unrelated, correct.
Anyways, good to have it!
##########
lucene/core/src/java/org/apache/lucene/internal/tests/TestSecrets.java:
##########
@@ -136,7 +150,21 @@ private static void ensureNull(Object ob) {
}
}
- private static void ensureCaller() {
+ private static void ensureCallerForSetter(Class<?> allowedCaller) {
+ final boolean validCaller =
+ StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE)
Review Comment:
Change to something like:
```java
final boolean validCaller =
StackWalker.getInstance()
.walk(
s ->
s.skip(2)
.limit(1)
.map(StackFrame::getClassName)
.allMatch(c -> Objects.equals(c,
allowedCaller.getName())));
if (!validCaller) {
throw new UnsupportedOperationException(
"The accessor can only be set by " + allowedCaller.getName() +
".");
}
```
##########
lucene/core/src/java/org/apache/lucene/internal/tests/TestSecrets.java:
##########
@@ -136,7 +150,21 @@ private static void ensureNull(Object ob) {
}
}
- private static void ensureCaller() {
+ private static void ensureCallerForSetter(Class<?> allowedCaller) {
+ final boolean validCaller =
+ StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE)
Review Comment:
We don't want to use `StackWalker.Option.RETAIN_CLASS_REFERENCE` in Lucene
code, especially not in those code paths that are non-tests, because in Java 21
SecurityManager is still active and requires higher privileges.
Therefor remove this and better just comare classname as string (like below
in the original code).
##########
lucene/core/src/test/org/apache/lucene/internal/tests/TestTestSecrets.java:
##########
@@ -37,4 +46,70 @@ public void testCannotSet() {
expectThrows(AssertionError.class, () ->
TestSecrets.setIndexPackageAccess(null));
expectThrows(AssertionError.class, () ->
TestSecrets.setSegmentReaderAccess(null));
}
+
+ public void testDeadlock() throws Exception {
Review Comment:
This test is fine, but I'd like to avoaid new tests spawning childs
processes as much as possible. It is good to reproduce, but maybe @dweiss has
an opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
