You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "benwtrent (via GitHub)" <gi...@apache.org> on 2024/08/02 01:31:05 UTC
[PR] Fix race condition on flush for DWPT seqNo generation [lucene]
benwtrent opened a new pull request, #13627:
URL: https://github.com/apache/lucene/pull/13627
There is a tricky race condition with DWPT threads. It is possible that a flush starts by advancing the deleteQueue (in charge of creating seqNo). Thus, the referenced deleteQueue, there should be a cap on the number of actions left.
However, it is possible after the advance, but before the DWPT are actually marked for flush, the DWPT gets freed and taken again to be used.
To replicate this extreme behavior, see: https://github.com/apache/lucene/compare/main...benwtrent:lucene:test-replicate-and-debug-13127?expand=1
This commit will prevent DWPT from being added back to the free list if their queue has been advanced. This is because the `maxSeqNo` for that queue was created accounting only for the current number of active threads. If the thread gets passed out again and still references the already advanced queue, it is possible that seqNo actually advances past the set `maxSeqNo`.
closes: https://github.com/apache/lucene/issues/13127
closes: https://github.com/apache/lucene/issues/13571
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1705880002
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java:
##########
@@ -430,10 +430,16 @@ long updateDocuments(
}
flushingDWPT = flushControl.doAfterDocument(dwpt);
} finally {
- if (dwpt.isFlushPending() || dwpt.isAborted()) {
- dwpt.unlock();
- } else {
- perThreadPool.marksAsFreeAndUnlock(dwpt);
+ // If a flush is occurring, we don't want to allow this dwpt to be reused
+ // If it is aborted, we shouldn't allow it to be reused
+ // If the deleteQueue is advanced, this means the maximum seqNo has been set and it cannot be
+ // reused
+ synchronized (flushControl) {
Review Comment:
Without this synchronization, I had a test failure where we would trip the new assertions in `marksAsFreeAndUnlock`
This seemed like an obvious race condition to me as we check all these conditions before suggesting the dwpt is available to be used again.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1705881356
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java:
##########
@@ -138,6 +138,15 @@ private synchronized boolean contains(DocumentsWriterPerThread state) {
void marksAsFreeAndUnlock(DocumentsWriterPerThread state) {
final long ramBytesUsed = state.ramBytesUsed();
+ assert state.isFlushPending() == false
+ && state.isAborted() == false
+ && state.isQueueAdvanced() == false
+ : "DWPT has pending flush: "
+ + state.isFlushPending()
+ + " aborted="
+ + state.isAborted()
+ + " queueAdvanced="
+ + state.isQueueAdvanced();
Review Comment:
The only place where we call this method is also where we check ALL these conditions. It just seemed prudent to add these assertions as the call flow obviously expects this.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1705717977
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java:
##########
@@ -140,6 +140,11 @@ void marksAsFreeAndUnlock(DocumentsWriterPerThread state) {
final long ramBytesUsed = state.ramBytesUsed();
assert contains(state)
: "we tried to add a DWPT back to the pool but the pool doesn't know about this DWPT";
+ // If we are closed we don't add the DWPT back to the free list. It has to be a "new" DWPT
+ if (state.deleteQueue.isAdvanced()) {
Review Comment:
Also I wonder if we should rather fix it at the call site in `DocumentsWriter`. It currently looks like this:
```java
if (dwpt.isFlushPending() || dwpt.isAborted()) {
dwpt.unlock();
} else {
perThreadPool.marksAsFreeAndUnlock(dwpt);
}
```
Since it already takes care of pending flushes and aborted segments, it would be more consistent to check if the queue is advanced there, e.g. something like this?
```java
if (dwpt.isFlushPending() || dwpt.isAborted() || dwpt.isQueueAdvanced()) {
dwpt.unlock();
} else {
perThreadPool.marksAsFreeAndUnlock(dwpt);
}
```
Relatedly, this makes me wonder if there is another way to fix the bug, by somehow marking DWPTs for flush (which prevents them from being reused) before advancing the queue in `DocumentsWriterFlushControl#markForFullFlush`.
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java:
##########
@@ -140,6 +140,11 @@ void marksAsFreeAndUnlock(DocumentsWriterPerThread state) {
final long ramBytesUsed = state.ramBytesUsed();
assert contains(state)
: "we tried to add a DWPT back to the pool but the pool doesn't know about this DWPT";
+ // If we are closed we don't add the DWPT back to the free list. It has to be a "new" DWPT
+ if (state.deleteQueue.isAdvanced()) {
Review Comment:
Do we need to make `DocumentsWriterDeleteQueue#isAdvanced` synchronized for correctness? I see that writes to `DocumentsWriterDeleteQueue#advanced` are protected but not reads via `isAdvanced`.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1706129153
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java:
##########
@@ -430,10 +430,16 @@ long updateDocuments(
}
flushingDWPT = flushControl.doAfterDocument(dwpt);
} finally {
- if (dwpt.isFlushPending() || dwpt.isAborted()) {
- dwpt.unlock();
- } else {
- perThreadPool.marksAsFreeAndUnlock(dwpt);
+ // If a flush is occurring, we don't want to allow this dwpt to be reused
+ // If it is aborted, we shouldn't allow it to be reused
+ // If the deleteQueue is advanced, this means the maximum seqNo has been set and it cannot be
+ // reused
+ synchronized (flushControl) {
Review Comment:
I played with luceneutil's IndexGeoNames which I've found historically found good at identifying contention at index time. It goes from ~17s (30 threads) to ~20s on my machine, which I suspect is due to this new lock. If it was needed, it would make sense to me (who cares about performance if it's incorrect) but I suspect that it is only needed here to make the assertion in `marksAsFreeAndUnlock` happy?
If I read your previous comments correctly, the point of this assertion is that we cannot write a good test now that the check moved from `DocumentsWriterPerThreadPool` to `DocumentsWriter`. This makes me wonder if we should move all checks (`dwpt.isFlushPending() || dwpt.isAborted() || dwpt.isQueueAdvanced()`) to `DocumentsWriterPerThreadPool` for testability purposes. (Sorry, I realize that this is the opposite from my previous suggestion.)
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1705834154
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java:
##########
@@ -140,6 +140,11 @@ void marksAsFreeAndUnlock(DocumentsWriterPerThread state) {
final long ramBytesUsed = state.ramBytesUsed();
assert contains(state)
: "we tried to add a DWPT back to the pool but the pool doesn't know about this DWPT";
+ // If we are closed we don't add the DWPT back to the free list. It has to be a "new" DWPT
+ if (state.deleteQueue.isAdvanced()) {
Review Comment:
@jpountz I am adding some asserts to `marksAsFreeAndUnlock` to ensure we aren't calling it if we are pending a flush or aborted or if the queue was advanced. This actually shows another race condition where we call this method sometimes if we are already pending a flush.
I will push up a fix for this as well.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1706831238
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java:
##########
@@ -430,10 +430,16 @@ long updateDocuments(
}
flushingDWPT = flushControl.doAfterDocument(dwpt);
} finally {
- if (dwpt.isFlushPending() || dwpt.isAborted()) {
- dwpt.unlock();
- } else {
- perThreadPool.marksAsFreeAndUnlock(dwpt);
+ // If a flush is occurring, we don't want to allow this dwpt to be reused
+ // If it is aborted, we shouldn't allow it to be reused
+ // If the deleteQueue is advanced, this means the maximum seqNo has been set and it cannot be
+ // reused
+ synchronized (flushControl) {
Review Comment:
All this logic is way too opaque and complicated to fully understand all the moving parts for me.
I added my assertions under the assumption that the only method that called this made all those checks previously.
Without the synchronizing, we will 100% put a DWPT back in for reuse even with `isFlushPending` being true.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "s1monw (via GitHub)" <gi...@apache.org>.
s1monw commented on PR #13627:
URL: https://github.com/apache/lucene/pull/13627#issuecomment-2272816300
it might be that my memory is blurry but let me suggest a different way of doing this. When we mark for a full flush in DWPTFlushControl we do lock the DWPTThreadPool for new writers. Once we have done that we can go ahead and set a boolean (let's call it `stale`) on each of the existing DWPT that it's stale and should be flushed. Then instead of checking `if (perThread.deleteQueue == documentsWriter.deleteQueue) {` in ` DocumentsWriterPerThreadFlushControl obtainAndLock()` we can check for that `stale` and just free that DWPT such that it's not used again. It would then be the same single volatile read? maybe I miss something
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1706831238
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java:
##########
@@ -430,10 +430,16 @@ long updateDocuments(
}
flushingDWPT = flushControl.doAfterDocument(dwpt);
} finally {
- if (dwpt.isFlushPending() || dwpt.isAborted()) {
- dwpt.unlock();
- } else {
- perThreadPool.marksAsFreeAndUnlock(dwpt);
+ // If a flush is occurring, we don't want to allow this dwpt to be reused
+ // If it is aborted, we shouldn't allow it to be reused
+ // If the deleteQueue is advanced, this means the maximum seqNo has been set and it cannot be
+ // reused
+ synchronized (flushControl) {
Review Comment:
All this logic is way to opaque and complicated to fully understand all the moving parts for me.
I added my assertions under the assumption that the only method that called this made all those checks previously.
Without the synchronizing, we will 100% put a DWPT back in for reuse even with `isFlushPending` being true.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1705753409
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterPerThreadPool.java:
##########
@@ -140,6 +140,11 @@ void marksAsFreeAndUnlock(DocumentsWriterPerThread state) {
final long ramBytesUsed = state.ramBytesUsed();
assert contains(state)
: "we tried to add a DWPT back to the pool but the pool doesn't know about this DWPT";
+ // If we are closed we don't add the DWPT back to the free list. It has to be a "new" DWPT
+ if (state.deleteQueue.isAdvanced()) {
Review Comment:
@jpountz so, I tried adjusting `isFlushPending` to take into account `isQueueAdvanced` and that caused many other tests to fail. I am guessing this is because we do other work on the DWPT before we actually mark for flush.
As for synchronizing for advanced, definitely, we should do that. I can see about updating this PR.
I will see how I can test this.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1705880530
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterDeleteQueue.java:
##########
@@ -636,7 +636,7 @@ long getMaxSeqNo() {
}
/** Returns <code>true</code> if it was advanced. */
- boolean isAdvanced() {
+ synchronized boolean isAdvanced() {
Review Comment:
@jpountz synchronizing per your suggestion. Now isAdvanced and the action that can flip the boolean value are both synchronied.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1706826818
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java:
##########
@@ -430,10 +430,16 @@ long updateDocuments(
}
flushingDWPT = flushControl.doAfterDocument(dwpt);
} finally {
- if (dwpt.isFlushPending() || dwpt.isAborted()) {
- dwpt.unlock();
- } else {
- perThreadPool.marksAsFreeAndUnlock(dwpt);
+ // If a flush is occurring, we don't want to allow this dwpt to be reused
+ // If it is aborted, we shouldn't allow it to be reused
+ // If the deleteQueue is advanced, this means the maximum seqNo has been set and it cannot be
+ // reused
+ synchronized (flushControl) {
Review Comment:
@jpountz so we are saying it is ok for a DWPT to be marked as free and reusable when `isFlushPending` is true?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13627:
URL: https://github.com/apache/lucene/pull/13627#issuecomment-2273681251
OK, I have confirmed that @s1monw 's proposed fix does work:
```
DocumentsWriterPerThread obtainAndLock() {
while (closed == false) {
final DocumentsWriterPerThread perThread = perThreadPool.getAndLock();
- if (perThread.deleteQueue == documentsWriter.deleteQueue) {
+ if (perThread.deleteQueue.isAdvanced() == false) {
```
And removing the other `isAdvanced()` check in [DocumentsWriter.java](https://github.com/apache/lucene/pull/13627/files#diff-3fa694d9c65c09b06582aa1d76470b931d43f67af81dd7753de7cf15d3d79ed2) still fixes the race condition.
I can switch to that method if we want. I can also remove the assertions I added @jpountz
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13627:
URL: https://github.com/apache/lucene/pull/13627#discussion_r1707359678
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java:
##########
@@ -430,10 +430,16 @@ long updateDocuments(
}
flushingDWPT = flushControl.doAfterDocument(dwpt);
} finally {
- if (dwpt.isFlushPending() || dwpt.isAborted()) {
- dwpt.unlock();
- } else {
- perThreadPool.marksAsFreeAndUnlock(dwpt);
+ // If a flush is occurring, we don't want to allow this dwpt to be reused
+ // If it is aborted, we shouldn't allow it to be reused
+ // If the deleteQueue is advanced, this means the maximum seqNo has been set and it cannot be
+ // reused
+ synchronized (flushControl) {
Review Comment:
I agree it's too hard to reason about, especially the synchronization logic. Let's make this correct first, we can look into better ways later on.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent merged PR #13627:
URL: https://github.com/apache/lucene/pull/13627
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Fix race condition on flush for DWPT seqNo generation [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13627:
URL: https://github.com/apache/lucene/pull/13627#issuecomment-2273283552
@s1monw I think `deleteQueue.isAdvanced()` is pretty much already this "stale" flag. It indicates that we have solidified how many tasks should be finished and thus we should never reuse it.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org