search

elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
1.09k stars 24.83k forks source link

Fix up `SearchResponse` refcounting in rollup #104503

Open DaveCTurner opened 9 months ago

DaveCTurner commented 9 months ago

Today TransportRollupSearchAction uses respondAndRelease to complete a listener which calls TransportChannel#sendResponse directly, representing two decRef() calls. There's a corresponding incRef() call on various branches under processResponse (including the rather twisty RollupResponseTranslator) but it's not obviously correct. Could we improve this? Or at least add a little more javadoc to any non-refcount-neutral methods to document their behaviour?

Also I found that the rollup test suite (e.g. ./gradlew :x-pack:plugin:rollup:check) doesn't really check for refcounting correctness, presumably because it doesn't do enough work to trigger the GC that the LeakDetector needs. I suspect this isn't the only test suite that won't catch bugs in this area. Can we do something to make our tests catch this sort of stuff more reliably?

elasticsearchmachine commented 9 months ago

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine commented 3 months ago

Pinging @elastic/es-search-foundations (Team:Search Foundations)