Ensure validation of the reader context is executed first #61831
Conversation
This change makes sure that we validate the reader context (`SearchOperationListener#validateReaderContext) before any other operation. Relates elastic#61446
jimczi
added
>non-issue
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
| @@ -61,6 +61,7 @@ public LegacyReaderContext(long id, IndexService indexService, IndexShard indexS | |||
| if (searcher == null) { | |||
| Engine.Searcher delegate = searcherSupplier.acquireSearcher(source); | |||
| onClose = delegate::close; | |||
| // wrap the searcher so that closing is a noop, the actual closing happens when this context is closed | |||
There was a problem hiding this comment.
@jimczi I looked at this again (while investigating the perf issue). I think we need to double-check with synchronization before acquiring a searcher to avoid leaking it.
There was a problem hiding this comment.
Or we can just always addOnClose(delegate) instead.
There was a problem hiding this comment.
I found another issue while investigating the perf issue. We don't have a happens-before relation between the initial search and subsequent scroll requests. The searcher field needs to be volatile; otherwise, we might acquire and wrap searcher more than once. I've pushed 0c42f9c. Let me know if you are ok with this change.
|
@elasticmachine run elasticsearch-ci/packaging-sample-windows |
) This change makes sure that reader context is validated (`SearchOperationListener#validateReaderContext) before any other operation and that it is correctly recycled or removed at the end of the operation. This commit also fixes a race condition bug that would allocate the security reader for scrolls more than once. Relates elastic#61446 Co-authored-by: Nhat Nguyen <[email protected]>
This change makes sure that reader context is validated (`SearchOperationListener#validateReaderContext) before any other operation and that it is correctly recycled or removed at the end of the operation. This commit also fixes a race condition bug that would allocate the security reader for scrolls more than once. Relates #61446 Co-authored-by: Nhat Nguyen <[email protected]>
|
I've backported this in 4d528e9 |
This change makes sure that we validate the reader context (
SearchOperationListener#validateReaderContext)before any other operation. This bug was introduced in #61062 so I marked this PR as a non-issue since it does not appear in any released version.
Relates #61446
Relates #61062