diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 62a5a3cdf7a..bdc874c9c9d 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -299,6 +299,9 @@ Bug Fixes * SOLR-6393: TransactionLog replay performance on HDFS is very poor. (Mark Miller) +* SOLR-6268: HdfsUpdateLog has a race condition that can expose a closed HDFS FileSystem instance and should + close it's FileSystem instance if either inherited close method is called. (Mark Miller) + Optimizations --------------------- diff --git a/solr/core/src/java/org/apache/solr/update/HdfsTransactionLog.java b/solr/core/src/java/org/apache/solr/update/HdfsTransactionLog.java index e79f8bc2c50..73df56e6d3f 100644 --- a/solr/core/src/java/org/apache/solr/update/HdfsTransactionLog.java +++ b/solr/core/src/java/org/apache/solr/update/HdfsTransactionLog.java @@ -274,21 +274,12 @@ public class HdfsTransactionLog extends TransactionLog { try { synchronized (this) { fos.flushBuffer(); - - // we must flush to hdfs - // TODO: we probably don't need to - // hsync below if we do this - I - // think they are equivalent. - tlogOutStream.hflush(); } if (syncLevel == UpdateLog.SyncLevel.FSYNC) { - // Since fsync is outside of synchronized block, we can end up with a partial - // last record on power failure (which is OK, and does not represent an error... - // we just need to be aware of it when reading). - - //raf.getFD().sync(); tlogOutStream.hsync(); + } else { + tlogOutStream.hflush(); } } catch (IOException e) { @@ -304,18 +295,23 @@ public class HdfsTransactionLog extends TransactionLog { } synchronized (this) { - fos.flush(); - tlogOutStream.hflush(); - fos.close(); - - tlogOutStream.close(); + fos.flushBuffer(); } + + tlogOutStream.hflush(); + tlogOutStream.close(); - if (deleteOnClose) { - fs.delete(tlogFile, true); - } } catch (IOException e) { + log.error("Exception closing tlog.", e); throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e); + } finally { + if (deleteOnClose) { + try { + fs.delete(tlogFile, true); + } catch (IOException e) { + throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e); + } + } } } diff --git a/solr/core/src/java/org/apache/solr/update/HdfsUpdateLog.java b/solr/core/src/java/org/apache/solr/update/HdfsUpdateLog.java index 9f85fd94bf7..f4ff9890a1c 100644 --- a/solr/core/src/java/org/apache/solr/update/HdfsUpdateLog.java +++ b/solr/core/src/java/org/apache/solr/update/HdfsUpdateLog.java @@ -114,16 +114,18 @@ public class HdfsUpdateLog extends UpdateLog { } } + FileSystem oldFs = fs; + try { - if (fs != null) { - fs.close(); - } + fs = FileSystem.newInstance(new Path(dataDir).toUri(), getConf()); } catch (IOException e) { throw new SolrException(ErrorCode.SERVER_ERROR, e); } try { - fs = FileSystem.newInstance(new Path(dataDir).toUri(), getConf()); + if (oldFs != null) { + oldFs.close(); + } } catch (IOException e) { throw new SolrException(ErrorCode.SERVER_ERROR, e); } @@ -278,8 +280,14 @@ public class HdfsUpdateLog extends UpdateLog { @Override public void close(boolean committed) { - synchronized (this) { - super.close(committed); + close(committed, false); + } + + @Override + public void close(boolean committed, boolean deleteOnClose) { + try { + super.close(committed, deleteOnClose); + } finally { IOUtils.closeQuietly(fs); } }