BulkProcessor can deadlock when bulk requests fail #47599
Labels
Comments
|
Pinging @elastic/es-core-features (:Core/Features/Watcher) |
|
Thanks to @suxinglee for finding this issue and providing a test case on #46790. I logged this issue as reference point and will close out this issue as fixed by #41451 in 7.3.0. I will evaluate if Watcher needs to implement the workaround for older versions in a different PR. |
jakelandis
added a commit
to jakelandis/elasticsearch
that referenced
this issue
Oct 4, 2019
7.3.0 fixes this issue by changing the locking strategy in elastic#41451. However, that change is not part of 6.x and the change here is a minimal workaround to prevent the potential of deadlock. This change will no longer retry failed bulk requests that go through the BulkProcessor for Watcher. Specifically this removes the retry logic when adding Watcher history and Triggered watches when the Bulk request failed. Related elastic#47599
|
Thank you for your reply.
PS: Another workaround is not to set the bulk flush interval. |
jakelandis
added a commit
that referenced
this issue
Oct 7, 2019
7.3.0 fixes this issue by changing the locking strategy in #41451. However, that change is not part of 6.x and the change here is a minimal workaround to prevent the potential of deadlock. This change will no longer retry failed bulk requests that go through the BulkProcessor for Watcher. Specifically this removes the retry logic when adding Watcher history and Triggered watches when the Bulk request failed. Related #47599
suxinglee
added a commit
to suxinglee/elasticsearch
that referenced
this issue
Oct 14, 2019
suxinglee
added a commit
to suxinglee/elasticsearch
that referenced
this issue
Oct 14, 2019
|
@jakelandis |
|
Re-opening issue since as @suxinglee points out this can still happen if the documents that flush is flushing fail. In the original description and what is fixed in 7.3.0 is "normal requests" fail -> Flush kicks in but is blocked by the request that failured -> Retry tries to happens but is blocked by Flush. The more granular locking fixes that deadlock case. Here, the question is what happens if the "Flush request" fail (which grabs the lock and only slot in the scheduler), and the Retry attempts to kick in. Retry can't run because the one and only slot in the scheduler is consumed by Flush, but Flush is waiting on the Retry to finish. Hence the deadlock. The workarounds are the same, disable Flush (pass null) , or disable Retry (use @suxinglee - thanks again for the test that illustrates this. I am still weighing the options of increasing single scheduler number of slots vs. using separate schedulers for flush and retry. |
jakelandis
added a commit
to jakelandis/elasticsearch
that referenced
this issue
Oct 30, 2019
Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes elastic#47599 Note - elastic#41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
jakelandis
added a commit
that referenced
this issue
Oct 31, 2019
Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes #47599 Note - #41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
jakelandis
added a commit
to jakelandis/elasticsearch
that referenced
this issue
Nov 11, 2019
Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes elastic#47599 Note - elastic#41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
jakelandis
added a commit
to jakelandis/elasticsearch
that referenced
this issue
Nov 11, 2019
Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes elastic#47599 Note - elastic#41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
jakelandis
added a commit
to jakelandis/elasticsearch
that referenced
this issue
Nov 11, 2019
Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes elastic#47599 Note - elastic#41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
jakelandis
added a commit
that referenced
this issue
Nov 11, 2019
Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes #47599 Note - #41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
jakelandis
added a commit
that referenced
this issue
Nov 11, 2019
Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes #47599 Note - #41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
jakelandis
added a commit
that referenced
this issue
Nov 12, 2019
* Prevent deadlock by using separate schedulers (#48697) Currently the BulkProcessor class uses a single scheduler to schedule flushes and retries. Functionally these are very different concerns but can result in a dead lock. Specifically, the single shared scheduler can kick off a flush task, which only finishes it's task when the bulk that is being flushed finishes. If (for what ever reason), any items in that bulk fails it will (by default) schedule a retry. However, that retry will never run it's task, since the flush task is consuming the 1 and only thread available from the shared scheduler. Since the BulkProcessor is mostly client based code, the client can provide their own scheduler. As-is the scheduler would require at minimum 2 worker threads to avoid the potential deadlock. Since the number of threads is a configuration option in the scheduler, the code can not enforce this 2 worker rule until runtime. For this reason this commit splits the single task scheduler into 2 schedulers. This eliminates the potential for the flush task to block the retry task and removes this deadlock scenario. This commit also deprecates the Java APIs that presume a single scheduler, and updates any internal code to no longer use those APIs. Fixes #47599 Note - #41451 fixed the general case where a bulk fails and is retried that can result in a deadlock. This fix should address that case as well as the case when a bulk failure *from the flush* needs to be retried.
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
If the BulkProcessor results in failed bulk requests, they will be retried via the RetryHandler. In versions of Elasticsearch prior to 7.3.0 this can result in a deadlock.
The deadlock can happen due to the
Schedulerwhich is shared between theFlushandRetrylogic. The deadlock can happen because theScheduleris configured with 1 worker thread which can be blocked in theFlushmethod. TheFlushmethod is guarded by synchronized (BulkProcessor.this) , but the internalAdd(..) method is also blocked by the same (synchronized) monitor lock. What can happen is that a bulk request comes in throughinternalAddobtains the lock, the bulk request is sent and a failure occurs, so the retry logic kicks in. The scheduler thread is blocked in theFlushmethod due to theinternalAdd's hold on the synchronized block , so now when the retry attempts to schedule a retry, it can not because theFlushis blocking the only worker thread for the scheduler. So hereFlushcan not continue because it is waiting oninternalAddto finish, andinternalAddcan not finish because it waiting onRetry, butRetrycan not finish because it is waiting on a scheduler thread which it can not obtain because it is waiting onFlushto finish.The change in 7.3.0 fixes this issue because it is much more selective about exactly what is locked, and no longer wraps the execution of the bulk request inside the lock.
Until
7.3.0the only workaround is to set the BackOffPolicy toBackoffPolicy.noBackoff()so that theRetrydoes not kick in. The default backoff isBackoffPolicy.exponentialBackoff()which is used by Watcher and is not configurable and thus is susceptible to this bug pre7.3.0This is related to #41418 butthe fix for that does not fix this issue. This issue is fixed by #41451 in 7.3.0.
EDIT: see below for a case where this can still happen in [7.3.0-7.5.0) (if the flush documents themselves fail)
The text was updated successfully, but these errors were encountered: