http: CodecClient handle TCP end_stream #2743

lizan · 2018-03-06T23:35:24Z

Signed-off-by: Lizan Zhou zlizan@google.com

Description:
Http::CodecClient should know if the connection is remote closed before onEvent.

Risk Level: Low

Testing:
unit test, integration tests

Docs Changes:
N/A

Release Notes:
N/A

Fixes #2715

Signed-off-by: Lizan Zhou <zlizan@google.com>

mattklein123 · 2018-03-06T23:38:10Z

source/common/http/codec_client.cc

@@ -96,7 +96,8 @@ void CodecClient::onReset(ActiveRequest& request, StreamResetReason reason) {
  deleteRequest(request);
 }

-void CodecClient::onData(Buffer::Instance& data) {
+void CodecClient::onData(Buffer::Instance& data, bool end_stream) {
+  remote_closed_ |= end_stream;


This is definitely the correct fix, but AFAIK we don't use half close semantics for any HTTP code, so I don't think this will actually solve your problem in practice. @ggreenway?

Yeah, Network::ReadFilter::onData() should never pass end_stream==true on the http code. If you're seeing that, there's a bug somewhere, probably in ConnectionImpl.

Will do some test with real env that reproduced #2715, though I believe this at least make this if statement true, so it won't assign next pending request in HTTP1 conn pool.

https://github.com/envoyproxy/envoy/blob/master/source/common/network/connection_impl.cc#L435

We could consider I guess setting end_stream to true in this case, even for not half-closed connections. That might allow you to win the race in more cases, but I think that is a more involved change.

Can ConnectionImpl::onRead() result in a new request being sent out on the upstream connection (if we read the end of a response)? If not, then it shouldn't matter; the RemoteClose is raised right after onRead().

@ggreenway that's exactly why we're seeing 503s in #2715 (the code is here). Let me take more look on how I can fix this.

Did this exact case ever work better? Given that onData and onEvent were always separate calls, there was a choice of either calling onEvent(RemoteClose) first, which would mean an error on the response (because we didn't process all the response data yet), or calling onData() first and getting this error.

For HTTP/1.1, I don't think this ever worked any better, because upstream is supposed to set Connection: close as I keep saying into the void. For HTTP/1 this used to work OK because of https://github.com/envoyproxy/envoy/pull/2743/files#diff-181fe1d44460531b055c9b316ca2cd69R64.

I don't have any objection per say to attempting to handle this better by potentially setting end_stream = true in this case, but again as I've said a ton of times, this is not going to work out in your favor 100% of the time due to certain local/remote interleavings.

Sorry for the late response (was sick last week).

I ended up by making onData set end_stream = true even half-close is not enabled. This new parameter is not used widely yet so it should be safe. Letting a TCP read filter know if the byte is the last one (before connection close) is useful even it doesn't handle half-close semantics.

whether it's a regression fix or just a fix is somewhat irrelevant no ? envoy should handle "unexpectedly" closed connection without attributing the error to the next request?

…stream

Signed-off-by: Lizan Zhou <zlizan@google.com>

ggreenway · 2018-03-13T23:30:08Z

source/common/network/connection_impl.cc

    result.action_ = PostIoAction::Close;
  }

  read_end_stream_ |= result.end_stream_read_;
-  if (result.bytes_processed_ != 0 || result.end_stream_read_) {
+  if (result.bytes_processed_ != 0 || (enable_half_close_ && result.end_stream_read_)) {


If end_stream is delivered without any data, and you want this to be delivered, I think you want to revert this line. Otherwise end_stream==true will only be set if it arrives with some data (or enable_half_close_ is true).

end_stream==true will only be set if it arrives with some data (or enable_half_close_ is true).

This is intentional, since we have converted half-close to full-close above, raising onData(0-length buffer, true) for non-half-close read filter is not needed since it will be destroyed right after onReadReady.

ggreenway

Please add some test coverage to connection_impl for the new end_stream behavior.

Signed-off-by: Lizan Zhou <zlizan@google.com>

…stream

mattklein123 · 2018-03-14T00:24:36Z

include/envoy/network/filter.h

@@ -81,8 +81,7 @@ class ReadFilter {
  /**
   * Called when data is read on the connection.
   * @param data supplies the read data which may be modified.
-   * @param end_stream supplies whether this is the last byte on the connection. This will only
-   *        be set if the connection has half-close semantics enabled.
+   * @param end_stream supplies whether this is the last byte on the connection.


Can you make this comment a bit more clear? end_stream may not be set on the last byte in the connection, if the FIN arrives after the last byte on a split packet.

Signed-off-by: Lizan Zhou <zlizan@google.com>

ggreenway · 2018-03-14T02:53:23Z

include/envoy/network/filter.h

@@ -81,8 +81,9 @@ class ReadFilter {
  /**
   * Called when data is read on the connection.
   * @param data supplies the read data which may be modified.
-   * @param end_stream supplies whether this is the last byte on the connection. This will only
-   *        be set if the connection has half-close semantics enabled.
+   * @param end_stream supplies whether this is the last byte on the connection. It is possible that


I dislike the interface that's being created here. It's confusing to reason about, because we're saying that end_stream may or may not be delivered, depending on how the bytes are broken into packets.

I think we should guarantee delivery of end_stream, so that it's simple to reason about the interface.

I agree, but if we don't do this I really have no idea how we are going to "fix" the issue because of we supply an empty onData with end_stream as true, the original issue will still repro.

I still really think we should just not "fix" this at all...

Maybe I should make a separate PR for onRead interface change since the discussion is separate.

@ggreenway not only depending on how the bytes are broken into packets, connection may be closed for several reasons. We cannot guarantee delivery of end_stream, instead, Envoy makes it best effort to set end_stream when onData is called, even if half-close is not enabled. I can revert source/common/network/connection_impl.cc line 439 but it still does not guarantee anything.

@mattklein123 I partly agree that for the original issue the upstream is responsible. While Envoy can make the best effort to let read filter know when it read end_stream (ReadFilter change), and HTTP conn pool can use that, no?

@lizan I think we should always deliver end_stream if we can, ie if the connection is shutdown cleanly.

I think we revert line 439, and in the http handling, if you get onData(empty, true) you can ignore it.

@mattklein123 I partly agree that for the original issue the upstream is responsible. While Envoy can make the best effort to let read filter know when it read end_stream (ReadFilter change), and HTTP conn pool can use that, no?

Yes, but when we are spending this much time talking about this change, and the change is becoming increasingly risky, it seems not worth it to me.

@lizan I think we should always deliver end_stream if we can, ie if the connection is shutdown cleanly. I think we revert line 439, and in the http handling, if you get onData(empty, true) you can ignore it.

Yes I think this should work. It will "fix" the problem when we get FIN inline with the final data, but still always deliver an end_stream with zero data before closing the connection in the half close case. Though I really do worry that there are going to be other filters that won't correctly handle this.

One other option would be to add some type of function to connection such as sawRemoteClose() (similar to what we have on codec client), and allow codecClient to call this. Then we can keep the half close / end_stream logic as it currently is.

@lizan I think we should always deliver end_stream if we can, ie if the connection is shutdown cleanly. I think we revert line 439, and in the http handling, if you get onData(empty, true) you can ignore it.

Please see #2841, OTOH I think it might be better not deliver onData(0-length data, true) even half-close is enabled, and let filters rely on onEvent(RemoteClose) for that case. Since they are same and deliver onData doesn't let us win the race since no data is there.

One other option would be to add some type of function to connection such as sawRemoteClose() (similar to what we have on codec client), and allow codecClient to call this. Then we can keep the half close / end_stream logic as it currently is.

I thought this once though I don't want to add another place in addition to onData and onEvent handle remote close. It should be simpler.

mattklein123 · 2018-03-19T17:55:53Z

@lizan I'm going to close this for now while we sort out the primary API changes. Once that lands, let's reopen and revisit this change.

http: CodecClient handle TCP end_stream

ab769f5

Signed-off-by: Lizan Zhou <zlizan@google.com>

mattklein123 reviewed Mar 6, 2018

View reviewed changes

htuch assigned ggreenway and alyssawilk Mar 7, 2018

lizan added 2 commits March 13, 2018 15:32

Merge remote-tracking branch 'upstream/master' into codec_client_end_…

90f3148

…stream

set end_stream for non-half-close onData

581fd96

Signed-off-by: Lizan Zhou <zlizan@google.com>

ggreenway reviewed Mar 13, 2018

View reviewed changes

ggreenway requested changes Mar 13, 2018

View reviewed changes

lizan added 2 commits March 13, 2018 17:21

test end_stream

30a8f61

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into codec_client_end_…

606eb05

…stream

mattklein123 reviewed Mar 14, 2018

View reviewed changes

fix comment

1174d35

Signed-off-by: Lizan Zhou <zlizan@google.com>

ggreenway requested changes Mar 14, 2018

View reviewed changes

lizan mentioned this pull request Mar 16, 2018

network: deliver onData with end_stream to ReadFilter if possible #2841

Closed

mattklein123 closed this Mar 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: CodecClient handle TCP end_stream #2743

http: CodecClient handle TCP end_stream #2743

lizan commented Mar 6, 2018

mattklein123 Mar 6, 2018

ggreenway Mar 6, 2018

lizan Mar 6, 2018

mattklein123 Mar 6, 2018

ggreenway Mar 6, 2018

lizan Mar 6, 2018

ggreenway Mar 7, 2018

mattklein123 Mar 7, 2018

lizan Mar 13, 2018

ldemailly Mar 20, 2018

ggreenway Mar 13, 2018

lizan Mar 14, 2018

ggreenway left a comment

mattklein123 Mar 14, 2018

lizan Mar 14, 2018

ggreenway Mar 14, 2018

mattklein123 Mar 14, 2018

lizan Mar 14, 2018

ggreenway Mar 14, 2018

mattklein123 Mar 14, 2018

lizan Mar 16, 2018

mattklein123 commented Mar 19, 2018

http: CodecClient handle TCP end_stream #2743

http: CodecClient handle TCP end_stream #2743

Conversation

lizan commented Mar 6, 2018

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

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

ggreenway 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Mar 19, 2018