Skip to content

[8.17] Fix NPE caused by race condition in async search when minimise round trips is true (#117504)#120955

Merged
elasticsearchmachine merged 1 commit intoelastic:8.17from
pawankartik-elastic:backport/8.17/pr-117504
Jan 27, 2025
Merged

[8.17] Fix NPE caused by race condition in async search when minimise round trips is true (#117504)#120955
elasticsearchmachine merged 1 commit intoelastic:8.17from
pawankartik-elastic:backport/8.17/pr-117504

Conversation

@pawankartik-elastic
Copy link
Copy Markdown
Contributor

Backports the following commits to 8.17:

…trips is true (elastic#117504)

* Fix NPE caused by race condition in async search when minimise round trips is true

Previously, the `notifyListShards()` initialised and updated the
required pre-requisites (`searchResponse` being amongst them) when a
search op began. This function takes in arguments that contain
shard-specific details amongst others. Because this information is not
immediately available when the search begins, it is not immediately
called. In some specific cases, there can be a race condition that can
cause the pre-requisities (such as `searchResponse`) to be accessed
before they're initialised, causing an NPE.

This fix addresses the race condition by splitting the initialisation
and subsequent updation amongst 2 different methods. This way, the
pre-requisities are always initialised and do not lead to an NPE.

* Try: call `notifyListShards()` after `notifySearchStart()` when minimize round trips is true

* Add removed code comment

* Pass `Clusters` to `SearchTask` rather than using progress listener to
signify search start.

To prevent polluting the progress listener with unnecessary search
specific details, we now pass the `Clusters` object to `SearchTask` when
a search op begins. This lets `AsyncSearchTask` access it and use it to
initialise `MutableSearchResponse` appropriately.

* Use appropriate `clusters` object rather than re-building it

* Do not double set `mutableSearchResponse`

* Move mutable entities such as shard counts out of `MutableSearchResponse`

* Address PR review: revert moving out mutable entities from
`MutableSearchResponse`

* Update docs/changelog/117504.yaml

* Get rid of `SetOnce` for `searchResponse`

* Drop redundant check around shards count

* Add a test that calls `onListShards()` at last and clarify `updateShardsAndClusters()`'s comment

* Fix test: ref count

* Address review comment: rewrite comment and test
@pawankartik-elastic pawankartik-elastic added :Search Foundations/Search Catch all for Search Foundations >bug auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Jan 27, 2025
@elasticsearchmachine elasticsearchmachine merged commit 8c39fff into elastic:8.17 Jan 27, 2025
@pawankartik-elastic pawankartik-elastic deleted the backport/8.17/pr-117504 branch January 27, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport >bug :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.17.2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants