http: add onComplete to AsyncClient::StreamCallbacks

[goaway]

Description: Add onComplete callback for asymmetric cases where end_stream may not be bidirectionally observable.
Risk Level: Medium
Testing: Updated unit tests

Co-authored-by: Jose Ulises Nino Rivera jnino@lyft.com
Signed-off-by: Mike Schore mike.schore@gmail.com

[goaway]

To expand on this, receiving end_stream on a callback does not necessarily indicate the stream is closed; since locally the stream may still be open. While this is something of a special case, it's enough to warrant the bookkeeping we see here and elsewhere with local and remote flags.

If AsyncClient and friends are used from asymmetric code paths (sending stuff is somewhere separate from receiving stuff), it's not currently possible to infer when a stream is actually closed. Even in the case of bidirectional usage, since local and remote closure states are private, this bookkeeping must otherwise be replicated by anything that ties resources to stream lifecycle.

[goaway]

In the original PR I stated, "This isn't required for AsyncRequest, since a request is a complete entity and the local stream will be closed as soon as it is sent." This is technically incorrect, since a remote could in theory return a response with end_stream set prior to the request completing. This seems like much more of an edge case, but I've updated it in this PR as well.*

[mattklein123]

Thanks this makes sense to me. Please check CI.

/wait

[goaway]

Thanks for the feedback so far everyone. Will make updates. (Also still stepping through one failing test.)

[goaway]

(ready for review)

[mattklein123]

Thanks for working on this. This seems like a nice improvement. Some questions.

/wait-any

[mattklein123]

I don't think I follow how this is happening. What's the callstack when we get called after being already closed?

[goaway]

I'm inferring from tests, but I believe this can happen when the connection pool fails to provide a connection, and surfaces this via a locally-created 503 response.

[mattklein123]

But how does that cause closeLocal() to get called twice?

[mattklein123]

Again a bit hazy on how this happens. Can you provide more info?

[goaway]

In this case, it's possible when a callback synchronously calls stream->reset. There's also a unit test that covers this.

[mattklein123]

But if closes only happen in non-reset cases, why are we calling closeRemote() in the case of reset?

/wait-any

[goaway]

Let's assume the case covered by the unit test.
We're in encodeHeaders:

envoy/source/common/http/async_client_impl.cc

Line 89 in ef543d8

void AsyncStreamImpl::encodeHeaders(HeaderMapPtr&& headers, bool end_stream) {

stream_callbacks.onHeaders is called and synchronously resets the stream. reset calls resetStream which calls cleanup:

envoy/source/common/http/async_client_impl.cc

Line 190 in ef543d8

cleanup();

cleanup sets local and remote to be closed:

envoy/source/common/http/async_client_impl.cc

Line 180 in ef543d8

local_closed_ = remote_closed_ = true;

the callback finishes executing, but we're still in encodeHeaders, which looks to see if end_stream is true, and if it is, goes ahead and calls closeRemote:

envoy/source/common/http/async_client_impl.cc

Line 94 in ef543d8

closeRemote(end_stream);

This is largely existing behavior, which this PR preserves, except for the added guards to prevent "extra" closures/resets.

[mattklein123]

OK I see. That was my confusion that we are using local_closed_ and remote_closed_ also in the case of reset. IMO this is kind of confusing, so if you want to somehow split this logic out I would go for that. If not, can you potentially make the comments in these cases a lot more robust for the next reader to come along? I think that would be good enough for me. Thank's for explaining all of this.

/wait

[mattklein123]

Thanks for the additional comments, this makes sense to me. I will wait a little bit to merge in case @htuch or @lizan have any additional comments. Thank you!

[mattklein123]

Yeah I agree, I don't remember the history here. We should look at cleaning this up. cc @htuch @lizan in case they remember any of this history.