Reduce usage of transport-layer timeouts

[DaveCTurner]

Transport-layer timeouts are kinda trappy, particularly noting that they
do not (reliably) cancel the remote task or perform other necessary
cleanup. Really such behaviour should be the responsibility of the
caller rather than the transport layer itself.

This commit introduces an ActionListener#addTimeout utility to allow
adding timeout wrappers to arbitrary listeners, and uses it to replace
several transport-layer timeouts for requests that have no cancellation
functionality anyway.

Relates #123568

[elasticsearchmachine]

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

[DaveCTurner]

One slight difference in this approach is that timing out in the transport layer completes (and thus releases) the TransportResponseHandler itself, whereas with a caller-level timeout we keep hold of the entry in ResponseHandlers awaiting the response on the wire. The retained handler itself will hold a reference to the ElasticsearchTimeoutException, but no references to the underlying listener. Thus there's a little more memory overhead, but on the other hand it eliminates all the mysterious Transport response handler not found of id [xxx] log messages.

[joshua-adams-1]

Will this parameter ultimately be removed?

[DaveCTurner]

It is also used to select between the different channel types (see org.elasticsearch.transport.TransportRequestOptions.Type) so no we still need it. But the goal is to remove org.elasticsearch.transport.TransportRequestOptions#timeout once it's unused.

[DaveCTurner]

it eliminates all the mysterious Transport response handler not found of id [xxx] log messages.

NB it also eliminates log messages of the form "Received response for a request that has timed out, sent [{}/{}ms] ago, timed out [{}/{}ms] ago, action [{}], node [{}], id [{}]". I'm in two minds about this. On the one hand, this isn't really very useful to know - it's up to the caller to deal with the timeout as it sees fit, and if that means logging something then it can do so. But on the other hand it isn't totally useless to see that a bunch of different requests to a particular node are all taking longer than expected. We could reinstate this.

Thanks Josh, I'm going to keep this one off of my ready-to-merge list for now until we've had a chance to discuss it as a team.

[ywangd]

a caller-level timeout we keep hold of the entry in ResponseHandlers awaiting the response on the wire

If the response never comes back, how do we clean up the stale ResponseHandler?

[DaveCTurner]

If the response never comes back, how do we clean up the stale ResponseHandler?

Same way we deal with the vast majority of requests that have no transport-level timeout: eventually the connection will close for some reason (nothing lasts forever) and that completes any outstanding handlers with a NodeDisconnectedException.

TBC that'd be a pretty serious bug, we already aren't relying on these timeouts to avoid leaking handlers like this.

[ywangd]

What do you think if we do the similar thing but keeping it at the transport layer? I think the benefit is handling most changes in one place (TransportService) so that callers do not need concern themselves with common logics such as cancellation? Or do you see any timeout, regardless of its behaviour, at the transport layer as an issue of its own?

[DaveCTurner]

callers do not need concern themselves with common logics such as cancellation

The trouble is that callers still have to concern themselves with cancellation. If they don't handle cancellation properly, the receiving node will just keep on processing the request even though the sending node has given up and moved on. That's how many callers behave today anyway - indeed all the callers touched by this PR do exactly that. The transport-layer timeout implementation pretty much encourages this sort of mistake. I'm ok with callers deciding that this is what they want I guess but there's no need to encode this antipattern in the transport layer.

Timeouts are usually an end-to-end thing. We don't need individual node-level requests to time out, we need the overall request to time out and cancel any remaining child tasks in reaction.

Note that the transport-layer timeouts were implemented some time before task cancellation. If we had done cancellation first, I do not think we'd have added transport-layer timeouts. Note also that it will make the transport layer quite a bit simpler if we could tear out everything to do with these timeouts.