Skip to content

Commit 96f862d

Browse files
authored
remove IndexReader.registerParentReader() (#15002)
Remove this function and associated WeakhashMap: it is not needed any more. The tests still pass, user still gets ACE (just slightly different exception message). Closes #14999
1 parent 1669883 commit 96f862d

File tree

6 files changed

+5
-72
lines changed

6 files changed

+5
-72
lines changed

lucene/CHANGES.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,10 @@ Improvements
5050
global queue min-score checking. Improves performance and consistency. (Mike Sokolov, Dzung Bui,
5151
Ben Trent)
5252

53+
* GITHUB#15002: Remove synchronized WeakHashMap from IndexReader. This was added to help prevent
54+
SIGSEGV with the old unsafe "mmap hack", which has been replaced by MemorySegments.
55+
(Uwe Schindler, Robert Muir)
56+
5357
Optimizations
5458
---------------------
5559
* GITHUB#14011: Reduce allocation rate in HNSW concurrent merge. (Viliam Durina)

lucene/core/src/java/org/apache/lucene/index/BaseCompositeReader.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ protected BaseCompositeReader(R[] subReaders, Comparator<R> subReadersSorter) th
8484
starts[i] = (int) maxDoc;
8585
final IndexReader r = subReaders[i];
8686
maxDoc += r.maxDoc(); // compute maxDocs
87-
r.registerParentReader(this);
8887
}
8988

9089
if (maxDoc > IndexWriter.getActualMaxDocs()) {

lucene/core/src/java/org/apache/lucene/index/FilterLeafReader.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,6 @@ protected FilterLeafReader(LeafReader in) {
334334
throw new NullPointerException("incoming LeafReader must not be null");
335335
}
336336
this.in = in;
337-
in.registerParentReader(this);
338337
}
339338

340339
@Override

lucene/core/src/java/org/apache/lucene/index/IndexReader.java

Lines changed: 1 addition & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,7 @@
1818

1919
import java.io.Closeable;
2020
import java.io.IOException;
21-
import java.util.Collections;
2221
import java.util.List;
23-
import java.util.Set;
24-
import java.util.WeakHashMap;
2522
import java.util.concurrent.atomic.AtomicInteger;
2623
import org.apache.lucene.store.AlreadyClosedException;
2724

@@ -146,24 +143,6 @@ public interface ClosedListener {
146143
void onClose(CacheKey key) throws IOException;
147144
}
148145

149-
private final Set<IndexReader> parentReaders =
150-
Collections.synchronizedSet(
151-
Collections.newSetFromMap(new WeakHashMap<IndexReader, Boolean>()));
152-
153-
/**
154-
* Expert: This method is called by {@code IndexReader}s which wrap other readers (e.g. {@link
155-
* CompositeReader} or {@link FilterLeafReader}) to register the parent at the child (this reader)
156-
* on construction of the parent. When this reader is closed, it will mark all registered parents
157-
* as closed, too. The references to parent readers are weak only, so they can be GCed once they
158-
* are no longer in use.
159-
*
160-
* @lucene.experimental
161-
*/
162-
public final void registerParentReader(IndexReader reader) {
163-
ensureOpen();
164-
parentReaders.add(reader);
165-
}
166-
167146
/**
168147
* For test framework use only.
169148
*
@@ -173,18 +152,6 @@ protected void notifyReaderClosedListeners() throws IOException {
173152
// nothing to notify in the base impl
174153
}
175154

176-
private void reportCloseToParentReaders() throws IOException {
177-
synchronized (parentReaders) {
178-
for (IndexReader parent : parentReaders) {
179-
parent.closedByChild = true;
180-
// cross memory barrier by a fake write:
181-
parent.refCount.addAndGet(0);
182-
// recurse:
183-
parent.reportCloseToParentReaders();
184-
}
185-
}
186-
}
187-
188155
/** Expert: returns the current refCount for this reader */
189156
public final int getRefCount() {
190157
// NOTE: don't ensureOpen, so that callers can see
@@ -253,8 +220,7 @@ public final void decRef() throws IOException {
253220
final int rc = refCount.decrementAndGet();
254221
if (rc == 0) {
255222
closed = true;
256-
try (Closeable _ = this::reportCloseToParentReaders;
257-
Closeable _ = this::notifyReaderClosedListeners) {
223+
try (Closeable _ = this::notifyReaderClosedListeners) {
258224
doClose();
259225
}
260226
} else if (rc < 0) {

lucene/core/src/java/org/apache/lucene/index/ParallelLeafReader.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -196,7 +196,6 @@ public ParallelLeafReader(
196196
if (!closeSubReaders) {
197197
reader.incRef();
198198
}
199-
reader.registerParentReader(this);
200199
}
201200
}
202201

lucene/core/src/test/org/apache/lucene/index/TestReaderClosed.java

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import org.apache.lucene.store.Directory;
2626
import org.apache.lucene.tests.analysis.MockAnalyzer;
2727
import org.apache.lucene.tests.analysis.MockTokenizer;
28-
import org.apache.lucene.tests.index.OwnCacheKeyMultiReader;
2928
import org.apache.lucene.tests.index.RandomIndexWriter;
3029
import org.apache.lucene.tests.util.LuceneTestCase;
3130
import org.apache.lucene.tests.util.TestUtil;
@@ -81,39 +80,6 @@ public void test() throws Exception {
8180
}
8281
}
8382

84-
// LUCENE-3800
85-
public void testReaderChaining() throws Exception {
86-
assertTrue(reader.getRefCount() > 0);
87-
LeafReader wrappedReader = new ParallelLeafReader(getOnlyLeafReader(reader));
88-
89-
// We wrap with a OwnCacheKeyMultiReader so that closing the underlying reader
90-
// does not terminate the threadpool (if that index searcher uses one)
91-
IndexSearcher searcher = newSearcher(new OwnCacheKeyMultiReader(wrappedReader));
92-
93-
TermRangeQuery query = TermRangeQuery.newStringRange("field", "a", "z", true, true);
94-
searcher.search(query, 5);
95-
reader.close(); // close original child reader
96-
try {
97-
searcher.search(query, 5);
98-
} catch (Exception e) {
99-
AlreadyClosedException ace = null;
100-
for (Throwable t = e; t != null; t = t.getCause()) {
101-
if (t instanceof AlreadyClosedException) {
102-
ace = (AlreadyClosedException) t;
103-
}
104-
}
105-
if (ace == null) {
106-
throw new AssertionError("Query failed, but not due to an AlreadyClosedException", e);
107-
}
108-
assertEquals(
109-
"this IndexReader cannot be used anymore as one of its child readers was closed",
110-
ace.getMessage());
111-
} finally {
112-
// close executor: in case of wrap-wrap-wrapping
113-
searcher.getIndexReader().close();
114-
}
115-
}
116-
11783
@Override
11884
public void tearDown() throws Exception {
11985
dir.close();

0 commit comments

Comments
 (0)