Skip to content
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

Avoid self-deadlock in the translog #29520

Merged
merged 3 commits into from
Apr 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -583,12 +583,7 @@ public Operation readOperation(Location location) throws IOException {
if (current.generation == location.generation) {
// no need to fsync here the read operation will ensure that buffers are written to disk
// if they are still in RAM and we are reading onto that position
try {
return current.read(location);
} catch (final Exception ex) {
closeOnTragicEvent(ex);
throw ex;
}
return current.read(location);
} else {
// read backwards - it's likely we need to read on that is recent
for (int i = readers.size() - 1; i >= 0; i--) {
Expand All @@ -598,6 +593,9 @@ public Operation readOperation(Location location) throws IOException {
}
}
}
} catch (final Exception ex) {
closeOnTragicEvent(ex);
throw ex;
}
return null;
}
Expand Down Expand Up @@ -735,15 +733,28 @@ public boolean ensureSynced(Stream<Location> locations) throws IOException {
}
}

/**
* Closes the translog if the current translog writer experienced a tragic exception.
*
* Note that in case this thread closes the translog it must not already be holding a read lock on the translog as it will acquire a
* write lock in the course of closing the translog
*
* @param ex if an exception occurs closing the translog, it will be suppressed into the provided exception
*/
private void closeOnTragicEvent(final Exception ex) {
// we can not hold a read lock here because closing will attempt to obtain a write lock and that would result in self-deadlock
assert readLock.isHeldByCurrentThread() == false : Thread.currentThread().getName();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

if (current.getTragicException() != null) {
try {
close();
} catch (final AlreadyClosedException inner) {
// don't do anything in this case. The AlreadyClosedException comes from TranslogWriter and we should not add it as suppressed because
// will contain the Exception ex as cause. See also https://github.com/elastic/elasticsearch/issues/15941
/*
* Don't do anything in this case. The AlreadyClosedException comes from TranslogWriter and we should not add it as
* suppressed because it will contain the provided exception as its cause. See also
* https://github.com/elastic/elasticsearch/issues/15941.
*/
} catch (final Exception inner) {
assert (ex != inner.getCause());
assert ex != inner.getCause();
ex.addSuppressed(inner);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1812,7 +1812,6 @@ public void testTragicEventCanBeAnyException() throws IOException {
assertTrue(translog.getTragicException() instanceof UnknownException);
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/29509")
public void testFatalIOExceptionsWhileWritingConcurrently() throws IOException, InterruptedException {
Path tempDir = createTempDir();
final FailSwitch fail = new FailSwitch();
Expand Down