Rationalize ref-counting around ChannelActionListener #102551

original-brownbear · 2023-11-23T18:36:22Z

This is a bit of a follow-up to #102498 and is needed to enable the correct handling of search responses both when going over the wire and running node-local, in the same way the other change was needed to not lose a reference.

This listener (the channel action listener) was behaving differently from other listeners in that it would effectively result in decRef by 1 when invoked. This worked out so far by accounting for this fact in calling code but is not maintainable now that more and more ref-counted things are going to be passed into it => make it behave like any other listener and be neutral in respect to ref count by acquiring the reference that the channel will release.
Marking non-issue since there's no real bug here yet in production, but the behavior.

This listener was behaving differently from other listeners in that it would effectively result in decRef by 1 when invoked. This worked out so far by accounting for this fact in calling code but is not maintainable now that more and more ref-counted things are going to be passed into it => make it behave like any other listener and be neutral in respect to ref count by acquiring the reference that the channel will release.

elasticsearchmachine · 2023-11-23T18:36:46Z

Pinging @elastic/es-distributed (Team:Distributed)

original-brownbear · 2023-11-23T18:40:40Z

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java

@@ -378,7 +378,10 @@ public void onFailure(Exception e) {
                                null
                            ),
                            new SearchShardTask(123L, "", "", "", null, Collections.emptyMap()),
-                            result
+                            result.delegateFailure((l, r) -> {
+                                r.incRef();


This kind of hack is fine, no need to do anything fancy, we never use this transport action with a future in prod.

original-brownbear · 2023-11-23T18:41:15Z

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java

@@ -391,7 +394,7 @@ public void onFailure(Exception e) {
                            );
                            PlainActionFuture<FetchSearchResult> listener = new PlainActionFuture<>();
                            service.executeFetchPhase(req, new SearchShardTask(123L, "", "", "", null, Collections.emptyMap()), listener);
-                            listener.get().decRef();
+                            listener.get();


Same here, this doesn't need to return anything that has > 0 ref count, just using the future to wait on the action here.

original-brownbear · 2023-11-23T18:43:18Z

server/src/main/java/org/elasticsearch/search/SearchService.java

@@ -648,8 +650,27 @@ private IndexShard getShard(ShardSearchRequest request) {
        return indicesService.indexServiceSafe(request.shardId().getIndex()).getShard(request.shardId().id());
    }

-    private static <T> void runAsync(Executor executor, CheckedSupplier<T, Exception> executable, ActionListener<T> listener) {
-        executor.execute(ActionRunnable.supply(listener, executable));


This was the problem, we implicitly assumed that the listener would always count down the reference here. This held for all current implementations which saw a ChannelActionListener but broke once I tried to use this with the multi-search response which can either go to a ChannelActionListener or some other listeners (which would then have to be aware of the fact that they need to count-down by one which would make the code needlessly brittle.

original-brownbear · 2023-11-23T18:45:48Z

server/src/main/java/org/elasticsearch/search/SearchService.java

+            @Override
+            public void accept(ActionListener<T> l) throws Exception {
+                var res = executable.get();
+                try {


Also, following this pattern (instantiate -> try -> finally -> decRef) as closely as possible and not implicitly requiring listeners to ever count down a ref makes the code a lot easier to follow and extend (as can be seen in the test simplifications ... mod the futures :P).

DaveCTurner

Makes sense to me, although why are there not more consequences? Is it just that we've never had a meaningfully ref-counted response before?

…action-listener

original-brownbear · 2023-11-23T19:18:27Z

Is it just that we've never had a meaningfully ref-counted response before?

We haven't the search responses would be the first ones that have client and other (directly through the transport service) usage it seems.

original-brownbear · 2023-11-23T19:18:55Z

Actually hold that thought ... Ccr seems to be affected looking into it, sorry for the noise.

DaveCTurner · 2023-11-23T19:19:01Z

Gotcha, ok. The tests suggest there's more to do here :)

DaveCTurner · 2023-11-23T19:36:37Z

Lots of good .mustIncRef() candidates here too tho, can we get #102515 in first?

DaveCTurner · 2023-11-24T14:16:56Z

server/src/main/java/org/elasticsearch/action/support/ChannelActionListener.java

@@ -28,6 +28,7 @@ public ChannelActionListener(TransportChannel channel) {

    @Override
    public void onResponse(Response response) {
+        response.incRef(); // acquire reference that will be released by channel.sendResponse below


On reflection I think it'd be nicer to push this down a level or two and make TransportChannel#sendResponse refcount-neutral. In particular it's kinda weird that OutboundHandler#sendRequest is already refcount-neutral whilst OutboundHandler#sendResponse is not.

Yea it sucks but could we push that out to another PR? That's practically a rather big change it seems, just tried it. We have a number of spots outside of this listener where we call the channel.sendResponse() directly and I'd have to adjust all of those. That goes beyond the scope of what I'm trying to fix here. Note that this at least gets us closer to fixing the OutBoundHandler :) ok with you?

Yeah ok makes sense - I opened #102600 to make sure we don't forget

.../esql/compute/src/main/java/org/elasticsearch/compute/operator/exchange/ExchangeService.java

alex-spies · 2023-11-24T16:12:01Z

.../compute/src/test/java/org/elasticsearch/compute/operator/exchange/ExchangeServiceTests.java

-                        while (origResp.hasReferences()) {
-                            newResp.incRef();
-                            origResp.decRef();
-                        }


@dnhatn could you take a look at the ESQL changes in this PR? Esp. here, I'm not sure killing all references of the message was intended to be part of the test. (It validates ownership assumption, the channel should only be able to kill its "own" reference.)

The loop ensures that newResp has the same number of references as origResp. This change should be okay, assuming the test passes.

original-brownbear · 2023-11-24T17:22:25Z

Thanks @alex-spies for the help here, @DaveCTurner could you also give this another look. Shouldn't have changed outside the ESQL part from the last time you reviewed I think, thanks!

DaveCTurner

LGTM assuming CI is happy: I've not validated that this is everything that needs doing, but everything here looks good.

Suggested some mustIncRef usages, but we can do them in a followup if you'd prefer.

DaveCTurner · 2023-11-24T17:28:39Z

server/src/main/java/org/elasticsearch/action/support/ChannelActionListener.java

@@ -28,6 +28,7 @@ public ChannelActionListener(TransportChannel channel) {

    @Override
    public void onResponse(Response response) {
+        response.incRef(); // acquire reference that will be released by channel.sendResponse below


Suggested change

response.incRef(); // acquire reference that will be released by channel.sendResponse below

response.mustIncRef(); // acquire reference that will be released by channel.sendResponse below

DaveCTurner · 2023-11-24T17:28:52Z

server/src/main/java/org/elasticsearch/search/SearchService.java

@@ -686,6 +707,7 @@ private SearchPhaseResult executeQueryPhase(ShardSearchRequest request, SearchSh
                final RescoreDocIds rescoreDocIds = context.rescoreDocIds();
                context.queryResult().setRescoreDocIds(rescoreDocIds);
                readerContext.setRescoreDocIds(rescoreDocIds);
+                // inc-ref query result because we close the SearchContext that references it in this try-with-resources block
                context.queryResult().incRef();


Suggested change

context.queryResult().incRef();

context.queryResult().mustIncRef();

DaveCTurner · 2023-11-24T17:29:02Z

server/src/main/java/org/elasticsearch/search/SearchService.java

@@ -783,6 +805,7 @@ public void executeQueryPhase(QuerySearchRequest request, SearchShardTask task,
                    final RescoreDocIds rescoreDocIds = searchContext.rescoreDocIds();
                    queryResult.setRescoreDocIds(rescoreDocIds);
                    readerContext.setRescoreDocIds(rescoreDocIds);
+                    // inc-ref query result because we close the SearchContext that references it in this try-with-resources block
                    queryResult.incRef();


Suggested change

queryResult.incRef();

queryResult.mustIncRef();

DaveCTurner · 2023-11-24T17:29:08Z

server/src/main/java/org/elasticsearch/search/SearchService.java

@@ -866,6 +889,7 @@ public void executeFetchPhase(ShardFetchRequest request, SearchShardTask task, A
                    executor.success();
                }
                var fetchResult = searchContext.fetchResult();
+                // inc-ref fetch result because we close the SearchContext that references it in this try-with-resources block
                fetchResult.incRef();


Suggested change

fetchResult.incRef();

fetchResult.mustIncRef();

DaveCTurner · 2023-11-24T17:29:16Z

server/src/test/java/org/elasticsearch/search/SearchServiceTests.java

@@ -378,7 +378,10 @@ public void onFailure(Exception e) {
                                null
                            ),
                            new SearchShardTask(123L, "", "", "", null, Collections.emptyMap()),
-                            result
+                            result.delegateFailure((l, r) -> {
+                                r.incRef();


Suggested change

r.incRef();

r.mustIncRef();

dnhatn

ESQL part looks good to me.

dnhatn · 2023-11-24T18:08:52Z

.../compute/src/test/java/org/elasticsearch/compute/operator/exchange/ExchangeServiceTests.java

-                        while (origResp.hasReferences()) {
-                            newResp.incRef();
-                            origResp.decRef();
-                        }


The loop ensures that newResp has the same number of references as origResp. This change should be okay, assuming the test passes.

original-brownbear · 2023-11-24T18:19:19Z

Thanks so much David and Nhat! Will merge as is but will do the mustIncRef in a follow-up, promise! :)

…" (#102609) This reverts commit 6f72a1c #102551 , sadly this is still causing ESQL test failures and we need to fix ESQL some more before we can continue here.

…elastic#102551)" (elastic#102609)" This reverts commit fdf51ac.

…lastic#102638) * Revert "Revert "Rationalize ref-counting around ChannelActionListener (elastic#102551)" (elastic#102609)" This reverts commit fdf51ac. * Fix response ownership in ESQL

This listener was behaving differently from other listeners in that it would effectively result in decRef by 1 when invoked. This worked out so far by accounting for this fact in calling code but is not maintainable now that more and more ref-counted things are going to be passed into it => make it behave like any other listener and be neutral in respect to ref count by acquiring the reference that the channel will release. Co-authored-by: Alexander Spies <alexander.spies@elastic.co>

…c#102551)" (elastic#102609) This reverts commit 6f72a1c elastic#102551 , sadly this is still causing ESQL test failures and we need to fix ESQL some more before we can continue here.

…lastic#102638) * Revert "Revert "Rationalize ref-counting around ChannelActionListener (elastic#102551)" (elastic#102609)" This reverts commit fdf51ac. * Fix response ownership in ESQL

…elastic#102551)" (elastic#102609)" This reverts commit fdf51ac.

original-brownbear added >non-issue :Distributed/Network Http and internode communication implementations labels Nov 23, 2023

elasticsearchmachine added v8.12.0 Team:Distributed Meta label for distributed team labels Nov 23, 2023

original-brownbear mentioned this pull request Nov 23, 2023

[Meta] Make SearchResponse use pooled buffers for search hits #102030

Closed

78 tasks

original-brownbear commented Nov 23, 2023

View reviewed changes

original-brownbear requested a review from DaveCTurner November 23, 2023 18:43

original-brownbear commented Nov 23, 2023

View reviewed changes

DaveCTurner reviewed Nov 23, 2023

View reviewed changes

Merge remote-tracking branch 'elastic/main' into rationalize-channel-…

e818603

…action-listener

original-brownbear added 2 commits November 23, 2023 21:03

fix ccr

3d44833

onemore'

ffe188d

original-brownbear mentioned this pull request Nov 24, 2023

Make AbstractBlock extend AbstractRefCounted #102583

Closed

DaveCTurner reviewed Nov 24, 2023

View reviewed changes

DecRef responses sent in ESQL exchanges

c4d069c

DaveCTurner reviewed Nov 24, 2023

View reviewed changes

.../esql/compute/src/main/java/org/elasticsearch/compute/operator/exchange/ExchangeService.java Outdated Show resolved Hide resolved

DaveCTurner mentioned this pull request Nov 24, 2023

Make TransportChannel#sendResponse refcount-neutral #102600

Closed

Fix broken ownership in ExchangeServiceTest

5f91d5a

alex-spies requested a review from dnhatn November 24, 2023 16:09

alex-spies reviewed Nov 24, 2023

View reviewed changes

Close ExchangeResponse where we actually let go

8441476

original-brownbear requested a review from DaveCTurner November 24, 2023 17:21

DaveCTurner approved these changes Nov 24, 2023

View reviewed changes

dnhatn approved these changes Nov 24, 2023

View reviewed changes

original-brownbear merged commit 6f72a1c into elastic:main Nov 24, 2023
14 checks passed

original-brownbear deleted the rationalize-channel-action-listener branch November 24, 2023 18:19

original-brownbear mentioned this pull request Nov 24, 2023

Revert "Rationalize ref-counting around ChannelActionListener (#102551)" #102609

Merged

alex-spies added a commit to alex-spies/elasticsearch that referenced this pull request Nov 27, 2023

Revert "Revert "Rationalize ref-counting around ChannelActionListener (…

1468090

…elastic#102551)" (elastic#102609)" This reverts commit fdf51ac.

alex-spies mentioned this pull request Nov 27, 2023

Rationalize ref-counting around ChannelActionListener (2nd attempt) #102638

Merged

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Dec 3, 2023

Revert "Revert "Rationalize ref-counting around ChannelActionListener (…

23e6b84

…elastic#102551)" (elastic#102609)" This reverts commit fdf51ac.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rationalize ref-counting around ChannelActionListener #102551

Rationalize ref-counting around ChannelActionListener #102551

original-brownbear commented Nov 23, 2023

elasticsearchmachine commented Nov 23, 2023

original-brownbear Nov 23, 2023

original-brownbear Nov 23, 2023

original-brownbear Nov 23, 2023

original-brownbear Nov 23, 2023

DaveCTurner left a comment

original-brownbear commented Nov 23, 2023

original-brownbear commented Nov 23, 2023

DaveCTurner commented Nov 23, 2023

DaveCTurner commented Nov 23, 2023

DaveCTurner Nov 24, 2023

original-brownbear Nov 24, 2023

DaveCTurner Nov 24, 2023 •

edited

Loading

alex-spies Nov 24, 2023

dnhatn Nov 24, 2023

original-brownbear commented Nov 24, 2023

DaveCTurner left a comment

DaveCTurner Nov 24, 2023

DaveCTurner Nov 24, 2023

DaveCTurner Nov 24, 2023

DaveCTurner Nov 24, 2023

DaveCTurner Nov 24, 2023

dnhatn left a comment •

edited

Loading

dnhatn Nov 24, 2023

original-brownbear commented Nov 24, 2023

	response.incRef(); // acquire reference that will be released by channel.sendResponse below
	response.mustIncRef(); // acquire reference that will be released by channel.sendResponse below

	context.queryResult().incRef();
	context.queryResult().mustIncRef();

Rationalize ref-counting around ChannelActionListener #102551

Rationalize ref-counting around ChannelActionListener #102551

Conversation

original-brownbear commented Nov 23, 2023

elasticsearchmachine commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

original-brownbear commented Nov 23, 2023

original-brownbear commented Nov 23, 2023

DaveCTurner commented Nov 23, 2023

DaveCTurner commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Nov 24, 2023

DaveCTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

original-brownbear commented Nov 24, 2023

DaveCTurner Nov 24, 2023 •

edited

Loading

dnhatn left a comment •

edited

Loading