You are finding great stuff with this stress test driver! We really need this in the tests :)
On Feb 25, 2013, at 8:25 AM, Erick Erickson <[email protected]> wrote: > What do you think about me checking 4196 in and opening a separate JIRA for > this issue? I think that is fine - I don't think the deadlock is related to 4196. I think the following patch will address it: Index: solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java =================================================================== --- solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java (revision 1450011) +++ solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java (working copy) @@ -348,10 +348,13 @@ // currently for testing purposes. Do a delete of complete index w/o worrying about versions, don't log, clean up most state in update log, etc if (delAll && cmd.getVersion() == -Long.MAX_VALUE) { - synchronized (solrCoreState.getUpdateLock()) { + solrCoreState.getUpdateLock().lock(); + try { deleteAll(); ulog.deleteAll(); return; + } finally { + solrCoreState.getUpdateLock().unlock(); } } @@ -361,7 +364,8 @@ // a realtime view of the index. When a new searcher is opened after a DBQ, that // flag can be cleared. If those thing happen concurrently, it's not thread safe. // - synchronized (solrCoreState.getUpdateLock()) { + solrCoreState.getUpdateLock().lock(); + try { if (delAll) { deleteAll(); } else { @@ -374,6 +378,8 @@ } if (ulog != null) ulog.deleteByQuery(cmd); + } finally { + solrCoreState.getUpdateLock().unlock(); } madeIt = true; @@ -397,7 +403,8 @@ Term idTerm = new Term(idField.getName(), cmd.getIndexedId()); // see comment in deleteByQuery - synchronized (solrCoreState.getUpdateLock()) { + solrCoreState.getUpdateLock().lock(); + try { RefCounted<IndexWriter> iw = solrCoreState.getIndexWriter(core); try { IndexWriter writer = iw.get(); @@ -412,6 +419,8 @@ } if (ulog != null) ulog.add(cmd, true); + } finally { + solrCoreState.getUpdateLock().unlock(); } } @@ -527,10 +536,13 @@ } if (!cmd.softCommit) { - synchronized (solrCoreState.getUpdateLock()) { // sync is currently needed to prevent preCommit - // from being called between preSoft and - // postSoft... see postSoft comments. + solrCoreState.getUpdateLock().lock(); // sync is currently needed to prevent preCommit + // from being called between preSoft and + // postSoft... see postSoft comments. + try { if (ulog != null) ulog.preCommit(cmd); + } finally { + solrCoreState.getUpdateLock().unlock(); } // SolrCore.verbose("writer.commit() start writer=",writer); @@ -557,14 +569,18 @@ if (cmd.softCommit) { // ulog.preSoftCommit(); - synchronized (solrCoreState.getUpdateLock()) { + solrCoreState.getUpdateLock().lock(); + try { if (ulog != null) ulog.preSoftCommit(cmd); core.getSearcher(true, false, waitSearcher, true); if (ulog != null) ulog.postSoftCommit(cmd); + } finally { + solrCoreState.getUpdateLock().unlock(); } // ulog.postSoftCommit(); } else { - synchronized (solrCoreState.getUpdateLock()) { + solrCoreState.getUpdateLock().lock(); + try { if (ulog != null) ulog.preSoftCommit(cmd); if (cmd.openSearcher) { core.getSearcher(true, false, waitSearcher); @@ -574,6 +590,8 @@ searchHolder.decref(); } if (ulog != null) ulog.postSoftCommit(cmd); + } finally { + solrCoreState.getUpdateLock().unlock(); } if (ulog != null) ulog.postCommit(cmd); // postCommit currently means new searcher has // also been opened @@ -713,8 +731,11 @@ // TODO: keep other commit callbacks from being called? // this.commit(cmd); // too many test failures using this method... is it because of callbacks? - synchronized (solrCoreState.getUpdateLock()) { + solrCoreState.getUpdateLock().lock(); + try { ulog.preCommit(cmd); + } finally { + solrCoreState.getUpdateLock().unlock(); } // todo: refactor this shared code (or figure out why a real CommitUpdateCommand can't be used) @@ -723,8 +744,11 @@ writer.setCommitData(commitData); writer.commit(); - synchronized (solrCoreState.getUpdateLock()) { + solrCoreState.getUpdateLock().lock(); + try { ulog.postCommit(cmd); + } finally { + solrCoreState.getUpdateLock().unlock(); } } } catch (Throwable th) { Index: solr/core/src/java/org/apache/solr/update/SolrCoreState.java =================================================================== --- solr/core/src/java/org/apache/solr/update/SolrCoreState.java (revision 1450012) +++ solr/core/src/java/org/apache/solr/update/SolrCoreState.java (working copy) @@ -37,12 +37,6 @@ public abstract class SolrCoreState { public static Logger log = LoggerFactory.getLogger(SolrCoreState.class); - private final Object deleteLock = new Object(); - - public Object getUpdateLock() { - return deleteLock; - } - private int solrCoreStateRefCnt = 1; public synchronized int getSolrCoreStateRefCnt() { @@ -76,6 +70,7 @@ } public abstract Lock getCommitLock(); + public abstract Lock getUpdateLock(); /** * Force the creation of a new IndexWriter using the settings from the given Index: solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java =================================================================== --- solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java (revision 1450012) +++ solr/core/src/java/org/apache/solr/update/DefaultSolrCoreState.java (working copy) @@ -56,6 +56,7 @@ private boolean writerFree = true; protected final ReentrantLock commitLock = new ReentrantLock(); + protected final ReentrantLock updateLock = new ReentrantLock(); public DefaultSolrCoreState(DirectoryFactory directoryFactory) { this.directoryFactory = directoryFactory; @@ -142,11 +143,16 @@ log.info("Waiting until IndexWriter is unused... core=" + coreName); boolean yieldedCommitLock = false; + boolean yieldedUpdateLock = false; try { if (commitLock.isHeldByCurrentThread()) { yieldedCommitLock = true; commitLock.unlock(); } + if (updateLock.isHeldByCurrentThread()) { + yieldedUpdateLock = true; + updateLock.unlock(); + } while (!writerFree) { try { @@ -161,6 +167,9 @@ if (yieldedCommitLock) { commitLock.lock(); } + if (yieldedUpdateLock) { + updateLock.lock(); + } } try { @@ -295,4 +304,9 @@ return commitLock; } + @Override + public Lock getUpdateLock() { + return updateLock; + } + } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
