Problem/Motivation

Certain search api backends like elasticsearch and opensearch require clearing the index if there are changes to field mappings. This requires calling clear in the updateIndex method of the BackendClient if mapping changes are detected. If a SearchApiException is thrown in the updateIndex method before clear is called, search api catches the exception and adds a pending task. When you go to execute the pending task, it tries to re-run updateIndex which then calls $index->clear() again. This then calls deleteAllIndexItems() on the server which again tries to run pending tasks for the server, which tries to run updateIndex again, which calls $index->clear() and so on...

The only way to fix is to manually delete the pending task.

Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'search_api.task.updateIndex') (Line: 212)
Drupal\search_api\Task\TaskManager->executeSpecificTask(Object) (Line: 264)
Drupal\search_api\Task\TaskManager->executeAllTasks(Array, 100) (Line: 95)
Drupal\search_api\Task\ServerTaskManager->execute(Object) (Line: 424)
Drupal\search_api\Entity\Server->deleteAllIndexItems(Object) (Line: 1222)
Drupal\search_api\Entity\Index->clear() (Line: 233)
Drupal\search_api_opensearch\SearchAPI\BackendClient->updateIndex(Object) (Line: 440)
Drupal\search_api_opensearch\Plugin\search_api\backend\OpenSearchBackend->updateIndex(Object) (Line: 160)
Drupal\search_api\Task\ServerTaskManager->executeTask(Object) (Line: 115)
Drupal\search_api\Task\ServerTaskManager->processEvent(Object, 'search_api.task.updateIndex', Object)
call_user_func(Array, Object, 'search_api.task.updateIndex', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'search_api.task.updateIndex') (Line: 212)
Drupal\search_api\Task\TaskManager->executeSpecificTask(Object) (Line: 264)
Drupal\search_api\Task\TaskManager->executeAllTasks(Array, 100) (Line: 95)
Drupal\search_api\Task\ServerTaskManager->execute(Object) (Line: 424)
Drupal\search_api\Entity\Server->deleteAllIndexItems(Object) (Line: 1222)
Drupal\search_api\Entity\Index->clear() (Line: 233)
Drupal\search_api_opensearch\SearchAPI\BackendClient->updateIndex(Object) (Line: 440)
Drupal\search_api_opensearch\Plugin\search_api\backend\OpenSearchBackend->updateIndex(Object) (Line: 160)
Drupal\search_api\Task\ServerTaskManager->executeTask(Object) (Line: 115)
Drupal\search_api\Task\ServerTaskManager->processEvent(Object, 'search_api.task.updateIndex', Object)
call_user_func(Array, Object, 'search_api.task.updateIndex', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'search_api.task.updateIndex') (Line: 212)
Drupal\search_api\Task\TaskManager->executeSpecificTask(Object) (Line: 362)
Drupal\search_api\Task\TaskManager->processBatch(Array, Array, Array) (Line: 297)

Steps to reproduce

  1. Throw a searchApiException in your backend client's updateIndex method.
  2. Remove the exception.
  3. The updateIndex method must have an $index->clear() method.
  4. Try "Execute pending tasks"
  5. You will get a loop.

Proposed resolution

The code that starts the pending task execution should remove the task from the list of pending tasks so that when $index->clear() is called again in updateIndex it doesn't try and run the pending task again.

Remaining tasks

N/A

Issue fork search_api-3543230

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

achap created an issue. See original summary.

drunken monkey made their first commit to this issue’s fork.

drunken monkey’s picture

Thanks a lot for reporting this problem, and sorry it took me a bit to respond!

I could indeed reproduce the problem, and it’s really not trivial. While it wouldn’t be too hard for the backend plugin to work around this problem (instead of $index->clear(), just call $index->reindex() and $this->deleteAllIndexItems() separately), we don’t want to force backend plugin developers to figure that out and do that – as far as possible, this should “just work”.

Unfortunately, we cannot just delete the task before executing it, otherwise we cannot guarantee any more that the task will be executed in case of something like a fatal error. We could implement a more elaborate system where workers “claim” tasks and we have those claims expire after some time (something like done here for the indexing queue) but that seems like a lot of work for this relatively small problem, plus potentially more error-prone. Also, this might mean that operations stop completely for some time if a task execution does fail.
(Though I now realize that it is currently possible that the same task will be executed multiple times concurrently, so that is also not ideal. But a different problem, one step at a time.)

It seems to me like the simpler and more robust solution would be to just prevent any nested task executions: When a task is currently being executed (i.e., during the event dispatch) we set some flag to prevent any parallel execution of tasks (both this same one and others). The problem would be then we’d then have to act as if all tasks had still been executed properly to allow the operation to go forward, which does seem a bit risky. On the other hand, it seems pretty sensible that pending tasks would not prevent us from doing any operations needed by a task currently being executed. So, logically, this should be fine, though this by no means guarantees that there wouldn’t be any problems in practice.

In any case, this approach would be implemented in this MR, please test/review!
Pipelines are passing so at least we shouldn’t be breaking something in an obvious way.

Making the restriction a bit more specific (i.e., only prevent execution of the same task, or only of tasks for the same server and/or index, etc.) would also seem like an option to prevent the specific problem you report, but I think logically we actually do want to ignore all pending tasks while executing one.

drunken monkey’s picture

Status: Active » Needs review
achap’s picture

Status: Needs review » Reviewed & tested by the community

Not a problem, I've actually been on holiday until Monday anyway so it worked out perfectly. The approach looks correct to me, I was also thinking of using a static variable to keep track of the currently active tasks but I'm not that familiar with the core search_api code and just checking if there is any active task seems simpler/better.

I also tested it alongside my patch in the related issue and it worked perfectly. No more infinite loop and the task completes successfully.

Vielen dank!

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Great to hear, thanks for reporting back!
Merged.
Thanks again!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • drunken monkey committed c371d048 on 8.x-1.x
    [#3543230] fix: Fixed infinite loop for some backends after updateIndex...
mparker17’s picture

I think that I've been running into this issue occasionally while working on the Elasticsearch Connector module's 8.0.x release series but I hadn't been able to track down a cause. Elasticsearch Connector is calling clear() in a function that gets called by BackendClient::updateIndex()!. @achap and @drunken monkey, thank you for tracking it down and fixing it!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.