Fix ref counting when using futures in AbstractClient #102498
Conversation
We fundamentally need to increment the ref count by one before assigning a ref counted value to a future result. Otherwise, trasnsport actions will have to understand the specific kind of listener they resolve and cannot decrement sent/consumed transport messages themselves.
original-brownbear
added
>non-issue
:Distributed/Network
elasticsearchmachine
added
Team:Distributed
|
Pinging @elastic/es-distributed (Team:Distributed) |
| @@ -37,7 +37,6 @@ protected <Request extends ActionRequest, Response extends ActionResponse> void | |||
| ActionListener<Response> listener | |||
| ) { | |||
| assertEquals(origin, threadPool().getThreadContext().getTransient(ThreadContext.ACTION_ORIGIN_TRANSIENT_NAME)); | |||
| super.doExecute(action, request, listener); | |||
There was a problem hiding this comment.
Neither here nor in the other test do we care about resolving the listener. The result passed by the noop client is always null. I figured this was still better than adding a null check or hackily casting a non-null response in the noop-client ...
| * on the result before it goes out of scope. | ||
| * @param <R> reference counted result type | ||
| */ | ||
| public static class ForRefCounted<R extends RefCounted> extends PlainActionFuture<R> { |
There was a problem hiding this comment.
We will probably need something similar for listenable future as well at some point. I didn't add any tests here yet before we're cool with the approach, I can though obviously it's trivial :)
There was a problem hiding this comment.
Hmm I see the problem but this is a fairly awful hack IMO. That said I don't see great alternatives without somewhat invasive changes. The whole concept of blocking calls to the client is kinda bad, but does have production users (mostly Watcher and Monitoring it seems).
For a general-purpose utility I think the whole future should implement Releasable so that there's a proper incRef/decRef pair to keep things clear. As written here, there's an implicit assumption that there's only one call to .get() the result: if there are multiple .get() callers then they would need to coordinate the single required decRef() call amongst themselves. That seems super-trappy to me.
Also for a general-purpose utility we'd need to handle the case where the future is completed multiple times, in which we only incRef the result if it's the first completion.
Maybe it'd be better to make this specific behaviour local to the client rather than a general-purpose utility. I expect we've more chance of being able to assert that there's only a single call to .get() the result in that case.
Maybe also it should be a different API on the client for those few actions where refs matter so that it's clear to the caller they are taking a ref when calling .get(). Not 100% sure about that tho.
|
@DaveCTurner I admit it, it's hacky :) How about this, I moved the custom future into the client to limit the blast radius and I promise I'll look into removing the production usage of blocking get?
|
There was a problem hiding this comment.
Yep I can live with this for now to get the search response work through. I left a suggestion for a slightly safer implementation. Could you also add something to assert that .get() is only called once? And we need to do something with the timed .get() because a timeout would be a leak, although I'm not sure what exactly would be best there.
server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java
Outdated
Show resolved
Hide resolved
|
Suggestion for a more general-purpose solution: #102507 |
|
@DaveCTurner I'll have a look later :) But for now, wdyt about this solution? I made it safer with your suggestion and added an assertion about this only getting called once. Good enough to unblock this one? :) |
|
I'm so sorry ... 🤦 cd4fdd5 |
|
#102515 would have saved some of the mockery muckery I think |
|
Thanks David! |
original-brownbear
added
the
auto-merge
Found this now that #102030 is getting closer to completion. It's fundamentally broken how the client deals with ref counted messages. We were only saved by the fact that currently we do do not handle any messages with non-noop ref counting through this client interface.
We fundamentally need to increment the ref count by one before assigning a ref counted value to a future result.
Otherwise, transport actions will have to understand the specific kind of listener they resolve and cannot decrement sent/consumed transport messages themselves.
marking non-issue since this hasn't caused any production trouble yet.