diff --git a/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java b/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java index 65f79e51b0ef..90533d3d9290 100644 --- a/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java +++ b/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java @@ -48,6 +48,7 @@ import java.util.function.BooleanSupplier; import java.util.function.IntPredicate; import java.util.stream.Collectors; +import java.util.stream.Stream; import java.util.stream.StreamSupport; import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.codecs.Codec; @@ -552,9 +553,8 @@ DirectoryReader getReader(boolean applyAllDeletes, boolean writeAllDeletes) thro release(rld); } }; - Closeable onGetReaderMergeResources = null; + Closeable onGetReaderMergeResources = () -> {}; SegmentInfos openingSegmentInfos = null; - boolean success2 = false; try { /* This is the essential part of the getReader method. We need to take care of the following things: * - flush all currently in-memory DWPTs to disk @@ -572,7 +572,6 @@ DirectoryReader getReader(boolean applyAllDeletes, boolean writeAllDeletes) thro * since once the flush is marked as done deletes start to be applied to the segments on disk without guarantees that * the corresponding added documents (in the update case) are flushed and visible when opening a SDR. */ - boolean success = false; synchronized (fullFlushLock) { try { // TODO: should we somehow make the seqNo available in the returned NRT reader? @@ -650,35 +649,29 @@ DirectoryReader getReader(boolean applyAllDeletes, boolean writeAllDeletes) thro stopCollectingMergedReaders.set(true); IOUtils.close( mergedReaders.values().stream() - .map( + .flatMap( sr -> - (Closeable) - () -> { - try { - deleter.decRef(sr.getSegmentInfo().files()); - } finally { - sr.close(); - } - }) + Stream.of( + () -> deleter.decRef(sr.getSegmentInfo().files()), sr)) .toList()); } }; } } - success = true; - } finally { - // Done: finish the full flush! - assert Thread.holdsLock(fullFlushLock); - docWriter.finishFullFlush(success); - if (success) { - processEvents(false); - doAfterFlush(); - } else { - if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "hit exception during NRT reader"); - } + } catch (Throwable t) { + // Uh oh, tidy up then rethrow + docWriter.finishFullFlush(false); + + if (infoStream.isEnabled("IW")) { + infoStream.message("IW", "hit exception during NRT reader: " + t); } + throw t; } + // Done: finish the full flush! + assert Thread.holdsLock(fullFlushLock); + docWriter.finishFullFlush(true); + processEvents(false); + doAfterFlush(); } if (onGetReaderMerges != null) { // only relevant if we do merge on getReader StandardDirectoryReader mergedReader = @@ -707,21 +700,19 @@ DirectoryReader getReader(boolean applyAllDeletes, boolean writeAllDeletes) thro if (infoStream.isEnabled("IW")) { infoStream.message("IW", "getReader took " + (System.currentTimeMillis() - tStart) + " ms"); } - success2 = true; - } catch (Error tragedy) { - tragicEvent(tragedy, "getReader"); - throw tragedy; - } finally { - if (!success2) { - try { - IOUtils.closeWhileHandlingException(r, onGetReaderMergeResources); - } finally { - maybeCloseOnTragicEvent(); - } - } else { - IOUtils.close(onGetReaderMergeResources); + } catch (Throwable t) { + if (t instanceof Error) { + onTragicEvent(t, "getReader"); } + try { + IOUtils.closeWhileSuppressingExceptions(t, r, onGetReaderMergeResources); + } finally { + // do the rollback, even if an Error is thrown during close above + maybeCloseOnTragicEvent(); + } + throw t; } + onGetReaderMergeResources.close(); return r; } @@ -955,7 +946,6 @@ public IndexWriter(Directory d, IndexWriterConfig conf) throws IOException { // we wrap with a sleeper and this might take some time. writeLock = d.obtainLock(WRITE_LOCK_NAME); - boolean success = false; try { directoryOrig = d; directory = new LockValidatingDirectoryWrapper(d, writeLock); @@ -1200,17 +1190,13 @@ public IndexWriter(Directory d, IndexWriterConfig conf) throws IOException { infoStream.message("IW", "init: create=" + create + " reader=" + reader); messageState(); } - - success = true; - - } finally { - if (!success) { - if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "init: hit exception on init; releasing write lock"); - } - IOUtils.closeWhileHandlingException(writeLock); - writeLock = null; + } catch (Throwable t) { + if (infoStream.isEnabled("IW")) { + infoStream.message("IW", "init: hit exception on init; releasing write lock: " + t); } + IOUtils.closeWhileSuppressingExceptions(t, writeLock); + writeLock = null; + throw t; } } @@ -1567,21 +1553,17 @@ private long updateDocuments( Iterable> docs) throws IOException { ensureOpen(); - boolean success = false; try { - final long seqNo = maybeProcessEvents(docWriter.updateDocuments(docs, delNode)); - success = true; - return seqNo; - } catch (Error tragedy) { - tragicEvent(tragedy, "updateDocuments"); - throw tragedy; - } finally { - if (success == false) { - if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "hit exception updating document"); - } - maybeCloseOnTragicEvent(); + return maybeProcessEvents(docWriter.updateDocuments(docs, delNode)); + } catch (Throwable t) { + if (t instanceof Error) { + onTragicEvent(t, "updateDocuments"); + } + if (infoStream.isEnabled("IW")) { + infoStream.message("IW", "hit exception updating document: " + t); } + maybeCloseOnTragicEvent(); + throw t; } } @@ -2637,7 +2619,6 @@ private void rollbackInternalNoCommit() throws IOException { public long deleteAll() throws IOException { ensureOpen(); // Remove any buffered docs - boolean success = false; /* hold the full flush lock to prevent concurrency commits / NRT reopens to * get in our way and do unnecessary work. -- if we don't lock this here we might * get in trouble if */ @@ -2689,16 +2670,12 @@ public long deleteAll() throws IOException { changeCount.incrementAndGet(); segmentInfos.changed(); globalFieldNumberMap.clear(); - success = true; - long seqNo = docWriter.getNextSequenceNumber(); - return seqNo; - } finally { - if (success == false) { - - if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "hit exception during deleteAll"); - } + return docWriter.getNextSequenceNumber(); + } catch (Throwable t) { + if (infoStream.isEnabled("IW")) { + infoStream.message("IW", "hit exception during deleteAll: " + t); } + throw t; } } } @@ -2829,11 +2806,7 @@ private synchronized long publishFrozenUpdates(FrozenBufferedUpdates packet) { // packet. The flush will retry this packet anyway to ensure all of them are applied tryApply(packet); } catch (Throwable t) { - try { - w.onTragicEvent(t, "applyUpdatesPacket"); - } catch (Throwable t1) { - t.addSuppressed(t1); - } + w.onTragicEvent(t, "applyUpdatesPacket"); throw t; } w.flushDeletesCount.incrementAndGet(); @@ -2955,18 +2928,13 @@ private void noDupDirs(Directory... dirs) { */ private List acquireWriteLocks(Directory... dirs) throws IOException { List locks = new ArrayList<>(dirs.length); - for (int i = 0; i < dirs.length; i++) { - boolean success = false; + for (Directory dir : dirs) { try { - Lock lock = dirs[i].obtainLock(WRITE_LOCK_NAME); - locks.add(lock); - success = true; - } finally { - if (success == false) { - // Release all previously acquired locks: - // TODO: addSuppressed? it could be many... - IOUtils.closeWhileHandlingException(locks); - } + locks.add(dir.obtainLock(WRITE_LOCK_NAME)); + } catch (Throwable t) { + // Release all previously acquired locks: + IOUtils.closeWhileSuppressingExceptions(t, locks); + throw t; } } return locks; @@ -3011,8 +2979,6 @@ public long addIndexes(Directory... dirs) throws IOException { Sort indexSort = config.getIndexSort(); - boolean successTop = false; - long seqNo; try { @@ -3047,7 +3013,6 @@ public long addIndexes(Directory... dirs) throws IOException { // Best-effort up front check: testReserveDocs(totalMaxDoc); - boolean success = false; try { for (SegmentInfos sis : commits) { for (SegmentCommitInfo info : sis) { @@ -3088,18 +3053,15 @@ public long addIndexes(Directory... dirs) throws IOException { infos.add(copySegmentAsIs(info, newSegName, context)); } } - success = true; - } finally { - if (!success) { - for (SegmentCommitInfo sipc : infos) { - // Safe: these files must exist - deleteNewFiles(sipc.files()); - } + } catch (Throwable t) { + for (SegmentCommitInfo sipc : infos) { + // Safe: these files must exist + deleteNewFiles(sipc.files()); } + throw t; } synchronized (this) { - success = false; try { ensureOpen(); @@ -3107,32 +3069,27 @@ public long addIndexes(Directory... dirs) throws IOException { reserveDocs(totalMaxDoc); seqNo = docWriter.getNextSequenceNumber(); - - success = true; - } finally { - if (!success) { - for (SegmentCommitInfo sipc : infos) { - // Safe: these files must exist - deleteNewFiles(sipc.files()); - } + } catch (Throwable t) { + for (SegmentCommitInfo sipc : infos) { + // Safe: these files must exist + deleteNewFiles(sipc.files()); } + throw t; } segmentInfos.addAll(infos); checkpoint(); } - - successTop = true; - - } catch (Error tragedy) { - tragicEvent(tragedy, "addIndexes(Directory...)"); - throw tragedy; - } finally { - if (successTop) { - IOUtils.close(locks); - } else { - IOUtils.closeWhileHandlingException(locks); + } catch (Throwable t) { + try { + if (t instanceof Error) { + tragicEvent(t, "addIndexes(Directory...)"); + } + } finally { + IOUtils.closeWhileSuppressingExceptions(t, locks); } + throw t; } + IOUtils.close(locks); maybeMerge(); return seqNo; @@ -3568,8 +3525,6 @@ private SegmentCommitInfo copySegmentAsIs( newInfoPerCommit.setFieldInfosFiles(info.getFieldInfosFiles()); newInfoPerCommit.setDocValuesUpdatesFiles(info.getDocValuesUpdatesFiles()); - boolean success = false; - Set copiedFiles = new HashSet<>(); try { // Copy the segment's files @@ -3578,12 +3533,10 @@ private SegmentCommitInfo copySegmentAsIs( directory.copyFrom(info.info.dir, file, newFileName, context); copiedFiles.add(newFileName); } - success = true; - } finally { - if (!success) { - // Safe: these files must exist - deleteNewFiles(copiedFiles); - } + } catch (Throwable t) { + // Safe: these files must exist + deleteNewFiles(copiedFiles); + throw t; } assert copiedFiles.equals(newInfoPerCommit.files()) @@ -3692,8 +3645,7 @@ private long prepareCommitInternal() throws IOException { try { synchronized (fullFlushLock) { - boolean flushSuccess = false; - boolean success = false; + boolean flushed = false; try { seqNo = docWriter.flushAllThreads(); if (seqNo < 0) { @@ -3709,7 +3661,7 @@ private long prepareCommitInternal() throws IOException { // cannot pass triggerMerges=true here else it can lead to deadlock: processEvents(false); - flushSuccess = true; + flushed = true; applyAllDeletesAndUpdates(); synchronized (this) { @@ -3753,16 +3705,15 @@ private long prepareCommitInternal() throws IOException { toCommit, stopAddingMergedSegments::get, MergeTrigger.COMMIT, _ -> {}); } } - success = true; - } finally { - if (!success) { - if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "hit exception during prepareCommit"); - } + } catch (Throwable t) { + if (infoStream.isEnabled("IW")) { + infoStream.message("IW", "hit exception during prepareCommit: " + t); } + throw t; + } finally { assert Thread.holdsLock(fullFlushLock); // Done: finish the full flush! - docWriter.finishFullFlush(flushSuccess); + docWriter.finishFullFlush(flushed); doAfterFlush(); } } @@ -4282,9 +4233,8 @@ private boolean doFlush(boolean applyAllDeletes) throws IOException { doBeforeFlush(); testPoint("startDoFlush"); - boolean success = false; - try { + try { if (infoStream.isEnabled("IW")) { infoStream.message("IW", " start flush: applyAllDeletes=" + applyAllDeletes); infoStream.message("IW", " index before flush " + segString()); @@ -4317,19 +4267,17 @@ private boolean doFlush(boolean applyAllDeletes) throws IOException { synchronized (this) { writeReaderPool(applyAllDeletes); doAfterFlush(); - success = true; return anyChanges; } - } catch (Error tragedy) { - tragicEvent(tragedy, "doFlush"); - throw tragedy; - } finally { - if (!success) { - if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "hit exception during flush"); - } - maybeCloseOnTragicEvent(); + } catch (Throwable t) { + if (t instanceof Error) { + onTragicEvent(t, "doFlush"); + } + if (infoStream.isEnabled("IW")) { + infoStream.message("IW", "hit exception during flush: " + t); } + maybeCloseOnTragicEvent(); + throw t; } } @@ -4650,7 +4598,6 @@ private synchronized boolean commitMerge(MergePolicy.OneMerge merge, MergeState. assert merge.info.info.maxDoc() != 0 || dropSegment; if (mergedUpdates != null) { - boolean success = false; try { if (dropSegment) { mergedUpdates.dropChanges(); @@ -4659,12 +4606,10 @@ private synchronized boolean commitMerge(MergePolicy.OneMerge merge, MergeState. // segment is not yet live (only below do we commit it // to the segmentInfos): release(mergedUpdates, false); - success = true; - } finally { - if (!success) { - mergedUpdates.dropChanges(); - readerPool.drop(merge.info); - } + } catch (Throwable t) { + mergedUpdates.dropChanges(); + readerPool.drop(merge.info); + throw t; } } @@ -4941,17 +4886,14 @@ final void mergeInit(MergePolicy.OneMerge merge) throws IOException { // Make sure any deletes that must be resolved before we commit the merge are complete: bufferedUpdatesStream.waitApplyForMerge(merge.segments, this); - boolean success = false; try { _mergeInit(merge); - success = true; - } finally { - if (!success) { - if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "hit exception in mergeInit"); - } - mergeFinish(merge); + } catch (Throwable t) { + if (infoStream.isEnabled("IW")) { + infoStream.message("IW", "hit exception in mergeInit: " + t); } + mergeFinish(merge); + throw t; } } @@ -5371,8 +5313,6 @@ public int length() { } if (useCompoundFile) { - success = false; - Collection filesToRemove = merge.info.files(); // NOTE: Creation of the CFS file must be performed with the original // directory rather than with the merging directory, so that it is not @@ -5381,26 +5321,25 @@ public int length() { try { createCompoundFile( infoStream, trackingCFSDir, merge.info.info, context, this::deleteNewFiles); - success = true; } catch (Throwable t) { - synchronized (this) { - if (merge.isAborted()) { - // This can happen if rollback is called while we were building - // our CFS -- fall through to logic below to remove the non-CFS - // merged files: - if (infoStream.isEnabled("IW")) { - infoStream.message( - "IW", "hit merge abort exception creating compound file during merge"); + try { + synchronized (this) { + if (merge.isAborted()) { + // This can happen if rollback is called while we were building + // our CFS -- fall through to logic below to remove the non-CFS + // merged files: + if (infoStream.isEnabled("IW")) { + infoStream.message( + "IW", "hit merge abort exception creating compound file during merge: " + t); + } + return 0; + } else { + handleMergeException(t, merge); } - return 0; - } else { - handleMergeException(t, merge); } - } - } finally { - if (success == false) { + } finally { if (infoStream.isEnabled("IW")) { - infoStream.message("IW", "hit exception creating compound file during merge"); + infoStream.message("IW", "hit exception creating compound file during merge: " + t); } // Safe: these files must exist deleteNewFiles(merge.info.files()); @@ -5442,15 +5381,12 @@ public int length() { // creating CFS so that 1) .si isn't slurped into CFS, // and 2) .si reflects useCompoundFile=true change // above: - boolean success2 = false; try { codec.segmentInfoFormat().write(directory, merge.info.info, context); - success2 = true; - } finally { - if (!success2) { - // Safe: these files must exist - deleteNewFiles(merge.info.files()); - } + } catch (Throwable t) { + // Safe: these files must exist + deleteNewFiles(merge.info.files()); + throw t; } // TODO: ideally we would freeze merge.info here!! @@ -5671,18 +5607,15 @@ private void startCommit(final SegmentInfos toSync) throws IOException { // This call can take a long time -- 10s of seconds // or more. We do it without syncing on this: - boolean success = false; final Collection filesToSync; try { filesToSync = toSync.files(false); directory.sync(filesToSync); - success = true; - } finally { - if (!success) { - pendingCommitSet = false; - pendingCommit = null; - toSync.rollbackCommit(directory); - } + } catch (Throwable t) { + pendingCommitSet = false; + pendingCommit = null; + toSync.rollbackCommit(directory); + throw t; } if (infoStream.isEnabled("IW")) { @@ -5758,11 +5691,16 @@ public void onTragicEvent(Throwable tragedy, String location) { assert tragedy instanceof MergePolicy.MergeAbortedException == false; // How can it be a tragedy when nothing happened? assert tragedy != null; - if (infoStream.isEnabled("IW")) { - infoStream.message( - "IW", "hit tragic " + tragedy.getClass().getSimpleName() + " inside " + location); + + try { + if (infoStream.isEnabled("IW")) { + infoStream.message( + "IW", "hit tragic " + tragedy.getClass().getSimpleName() + " inside " + location); + } + this.tragedy.compareAndSet(null, tragedy); // only set it once + } catch (Throwable t) { + tragedy.addSuppressed(t); } - this.tragedy.compareAndSet(null, tragedy); // only set it once } /** @@ -5897,15 +5835,12 @@ static void createCompoundFile( infoStream.message("IW", "create compound file"); } // Now merge all added files - boolean success = false; try { info.getCodec().compoundFormat().write(directory, info, context); - success = true; - } finally { - if (!success) { - // Safe: these files must exist - deleteFiles.accept(directory.getCreatedFiles()); - } + } catch (Throwable t) { + // Safe: these files must exist + deleteFiles.accept(directory.getCreatedFiles()); + throw t; } // Replace all previous files with the CFS/CFE files: @@ -6290,15 +6225,16 @@ final void forceApply(FrozenBufferedUpdates updates) throws IOException { deleter.incRef(delFiles); } - AtomicBoolean success = new AtomicBoolean(); long delCount; - try (Closeable finalizer = () -> finishApply(segStates, success.get(), delFiles)) { - assert finalizer != null; // access the finalizer to prevent a warning + try { // don't hold IW monitor lock here so threads are free concurrently resolve // deletes/updates: delCount = updates.apply(segStates); - success.set(true); + } catch (Throwable t) { + finishApply(segStates, false, delFiles); + throw t; } + finishApply(segStates, true, delFiles); // Since we just resolved some more deletes/updates, now is a good time to write them: writeSomeDocValuesUpdates(); diff --git a/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java b/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java index b18da6234bbc..6ea1b2f5a6b1 100644 --- a/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java +++ b/lucene/core/src/java/org/apache/lucene/index/ReadersAndUpdates.java @@ -638,8 +638,6 @@ public synchronized boolean writeFieldUpdates( // Do this so we can delete any created files on // exception; this saves all codecs from having to do it: TrackingDirectoryWrapper trackingDir = new TrackingDirectoryWrapper(dir); - - boolean success = false; try { final Codec codec = info.info.getCodec(); @@ -696,20 +694,17 @@ public synchronized boolean writeFieldUpdates( reader.close(); } } - - success = true; - } finally { - if (success == false) { - // Advance only the nextWriteFieldInfosGen and nextWriteDocValuesGen, so - // that a 2nd attempt to write will write to a new file - info.advanceNextWriteFieldInfosGen(); - info.advanceNextWriteDocValuesGen(); - - // Delete any partially created file(s): - for (String fileName : trackingDir.getCreatedFiles()) { - IOUtils.deleteFilesIgnoringExceptions(dir, fileName); - } + } catch (Throwable t) { + // Advance only the nextWriteFieldInfosGen and nextWriteDocValuesGen, so + // that a 2nd attempt to write will write to a new file + info.advanceNextWriteFieldInfosGen(); + info.advanceNextWriteDocValuesGen(); + + // Delete any partially created file(s): + for (String fileName : trackingDir.getCreatedFiles()) { + IOUtils.deleteFilesSuppressingExceptions(t, dir, fileName); } + throw t; } // Prune the now-written DV updates: @@ -806,15 +801,12 @@ private SegmentReader createNewReaderWithLatestLiveDocs(SegmentReader reader) th pendingDeletes.getHardLiveDocs(), pendingDeletes.numDocs(), true); - boolean success2 = false; try { pendingDeletes.onNewReader(newReader, info); reader.decRef(); - success2 = true; - } finally { - if (success2 == false) { - newReader.decRef(); - } + } catch (Throwable t) { + newReader.decRef(); + throw t; } return newReader; }