s1monw commented on code in PR #12751:
URL: https://github.com/apache/lucene/pull/12751#discussion_r1382391250
##########
lucene/core/src/java/org/apache/lucene/index/IndexWriter.java:
##########
@@ -2560,10 +2560,15 @@ private void rollbackInternalNoCommit() throws
IOException {
// close all the closeables we can (but important is readerPool and
writeLock to prevent
// leaks)
- IOUtils.closeWhileHandlingException(readerPool, deleter, writeLock);
- writeLock = null;
- closed = true;
- closing = false;
+ try {
+ IOUtils.closeWhileHandlingException(readerPool, deleter,
writeLock);
+ } catch (Throwable t) {
+ throwable.addSuppressed(t);
+ } finally {
+ writeLock = null;
+ closed = true;
+ closing = false;
+ }
// So any "concurrently closing" threads wake up and see that the
close has now completed:
notifyAll();
Review Comment:
yeah you are right, this is also a recipe for deadlocks here... maybe we
should do something like this:
```diff
--- a/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
+++ b/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java
@@ -2463,7 +2463,15 @@ public class IndexWriter
if (infoStream.isEnabled("IW")) {
infoStream.message("IW", "rollback");
}
-
+ Closeable cleanupAndNotify =
+ () -> {
+ assert Thread.holdsLock(this);
+ writeLock = null;
+ closed = true;
+ closing = false;
+ // So any "concurrently closing" threads wake up and see that the
close has now completed:
+ notifyAll();
+ };
try {
synchronized (this) {
// must be synced otherwise register merge might throw and
exception if merges
@@ -2531,43 +2539,35 @@ public class IndexWriter
// after we leave this sync block and before we enter the sync
block in the finally clause
// below that sets closed:
closed = true;
-
- IOUtils.close(writeLock); // release write lock
- writeLock = null;
- closed = true;
- closing = false;
- // So any "concurrently closing" threads wake up and see that the
close has now completed:
- notifyAll();
+ IOUtils.close(writeLock, cleanupAndNotify); // release write lock
}
} catch (Throwable throwable) {
try {
// Must not hold IW's lock while closing
// mergeScheduler: this can lead to deadlock,
// e.g. TestIW.testThreadInterruptDeadlock
- IOUtils.closeWhileHandlingException(mergeScheduler);
- synchronized (this) {
- // we tried to be nice about it: do the minimum
- // don't leak a segments_N file if there is a pending commit
- if (pendingCommit != null) {
- try {
- pendingCommit.rollbackCommit(directory);
- deleter.decRef(pendingCommit);
- } catch (Throwable t) {
- throwable.addSuppressed(t);
- }
- pendingCommit = null;
- }
-
- // close all the closeables we can (but important is readerPool
and writeLock to prevent
- // leaks)
- IOUtils.closeWhileHandlingException(readerPool, deleter,
writeLock);
- writeLock = null;
- closed = true;
- closing = false;
-
- // So any "concurrently closing" threads wake up and see that the
close has now completed:
- notifyAll();
- }
+ IOUtils.closeWhileHandlingException(
+ mergeScheduler,
+ () -> {
+ synchronized (this) {
+ // we tried to be nice about it: do the minimum
+ // don't leak a segments_N file if there is a pending commit
+ if (pendingCommit != null) {
+ try {
+ pendingCommit.rollbackCommit(directory);
+ deleter.decRef(pendingCommit);
+ } catch (Throwable t) {
+ throwable.addSuppressed(t);
+ }
+ pendingCommit = null;
+ }
+ // close all the closeables we can (but important is
readerPool and writeLock to
+ // prevent
+ // leaks)
+ IOUtils.closeWhileHandlingException(
+ readerPool, deleter, writeLock, cleanupAndNotify);
+ }
+ });
} catch (Throwable t) {
throwable.addSuppressed(t);
} finally {
```
--
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]
