network: delayed conn close by AndresGuedez · Pull Request #4382 · envoyproxy/envoy

AndresGuedez · 2018-09-10T12:22:03Z

network: Support delayed close of downstream conns (#2929)

Mitigate client read/close race issues on downstream HTTP connections by adding a new connection
close type 'FlushWriteAndDelay'. This new close type flushes the write buffer on a connection but
does not immediately close after emptying the buffer (unlike ConnectionCloseType::FlushWrite).

A timer has been added to track delayed closes for both 'FlushWrite' and 'FlushWriteAndDelay'. Upon
triggering, the socket will be closed and the connection will be cleaned up.

Delayed close processing can be disabled by setting the newly added HCM 'delayed_close_timeout'
config option to 0.

Risk Level: Medium (changes common case behavior for closing of downstream HTTP connections)
Testing: Unit tests and integration tests added.
Fixes #2929.

Signed-off-by: Andres Guedez aguedez@google.com

Mitigate client read/close race issues on downstream HTTP connections by adding a new connection close type 'FlushWriteAndDelay'. This new close type flushes the write buffer on a connection but does not immediately close after emptying the buffer (unlike ConnectionCloseType::FlushWrite). A timer has been added to track delayed closes for both 'FlushWrite' and 'FlushWriteAndDelay'. Upon triggering, the socket will be closed and the connection will be cleaned up. Delayed close processing can be disabled by setting the newly added HCM 'delayed_close_timeout' config option to 0. Risk Level: Medium (changes common case behavior for closing of downstream HTTP connections) Testing: Unit tests and integration tests added. Fixes envoyproxy#2929. Signed-off-by: Andres Guedez <aguedez@google.com>

HTTP/2 integration tests added for delayed close processing. Signed-off-by: Andres Guedez <aguedez@google.com>

Signed-off-by: Andres Guedez <aguedez@google.com>

Increase the timeout values for the common case (no timeout trigerred) tests to prevent test flakiness. Signed-off-by: Andres Guedez <aguedez@google.com>

dnoe

Nice, thanks for fixing this! Mostly style nits.

dnoe · 2018-09-10T19:22:13Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  //
+  // A value of 0 will completely disable delayed close processing, and the downstream connection's
+  // socket will be closed immediately after the write flush is completed.
+  google.protobuf.Duration delayed_close_timeout = 25 [(gogoproto.stdduration) = true];


Is there a default value? What happens if delayed_close_timeout is not set?

There is a default timeout value of 1000 ms. I've amended the comment to note this.

dnoe · 2018-09-10T19:23:01Z

include/envoy/network/connection.h

  FlushWrite, // Flush pending write data before raising ConnectionEvent::LocalClose
-  NoFlush     // Do not flush any pending data and immediately raise ConnectionEvent::LocalClose
+  NoFlush,    // Do not flush any pending data and immediately raise ConnectionEvent::LocalClose
+  FlushWriteAndDelay


Can you add a similar comment to the above two lines?

dnoe · 2018-09-10T19:40:07Z

source/common/network/connection_impl.cc

-    close_with_flush_ = true;
+    RELEASE_ASSERT(type == ConnectionCloseType::FlushWrite ||
+                       type == ConnectionCloseType::FlushWriteAndDelay,
+                   "");


What's the rationale behind making this a RELEASE_ASSERT? What's the practical implication if it was an ASSERT and happened in production?

I'm using the RELEASE_ASSERT to provide runtime protection against bugs introduced in this logic, since they could lead to issues such as resource leaks and/or memory corruption.

I just re-read the style guide and it does mention that ASSERT should be the common case for this type of check, so I can certainly change it if that's the convention.

The convention is generally to use ASSERT for this, if you feel strongly about RELEASE_ASSERT I'll defer to senior maintainer for the decision.

Changed to ASSERT() per style guidelines.

dnoe · 2018-09-10T19:41:54Z

source/common/network/connection_impl.cc

+                       type == ConnectionCloseType::FlushWriteAndDelay,
+                   "");
+    delayed_close_ = true;
+    bool delayed_close_timeout_set = delayedCloseTimeout().count();


I'd prefer to see the explicit boolean expression here, ie, delayedCloseTimeout().count() > 0.

dnoe · 2018-09-10T19:43:44Z

source/common/network/connection_impl.cc

+
+    // Force a closeSocket() after the write buffer is flushed if the close_type calls for it or if
+    // no delayed close timeout is set.
+    close_after_flush_ = type == ConnectionCloseType::FlushWrite || !delayed_close_timeout_set;


Super nit, but I think this expression is more readable if you reverse the two halves of the or, since it separates the = and == more.

dnoe · 2018-09-10T20:07:32Z

test/common/network/connection_impl_test.cc

+  std::unique_ptr<Network::ConnectionImpl> server_connection(new Network::ConnectionImpl(
+      dispatcher, std::make_unique<ConnectionSocketImpl>(0, nullptr, nullptr),
+      TransportSocketPtr(transport_socket), true));
+  server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(500));


Are we safe from flakes here?

Yes. The timer's callback is manually invoked at the end of the test. The 'setDelayedCloseTimeout()' call is required to enable creation of the timer, but the actual value does not matter (as long as it's >0). I've added a clarifying comment.

dnoe · 2018-09-10T20:08:24Z

test/common/network/connection_impl_test.cc

+TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTimerTriggerTest) {
+  setUpBasicConnection();
+  connect();
+  server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(500));


Same question about flakes here. How do we know 500ms is the right number?

This is safe. Since the client connection does not issue a close() during this test, the timer always triggers. I've added a clarifying comment and reduced the timeout value since there is no need to wait so long.

dnoe · 2018-09-10T20:10:20Z

test/common/network/connection_impl_test.cc

 using testing::Invoke;
 using testing::InvokeWithoutArgs;
 using testing::Return;
+using testing::ReturnNew;


Is this used?

dnoe · 2018-09-10T20:18:21Z

test/integration/http2_integration_test.cc

+      version_);
+
+  connection.run();
+  EXPECT_TRUE(response.find("SETTINGS expected") != std::string::npos);


I find the form EXPECT_THAT(response, HasSubstr("SETTINGS expected")) more readable when trying to understand how the test works.

Thanks for pointing out the matcher. Done.

dnoe · 2018-09-10T20:21:44Z

test/integration/http2_integration_test.cc

+      version_);
+
+  connection.run();
+  EXPECT_TRUE(response.find("SETTINGS expected") != std::string::npos);


Same here, the HasSubstr matcher will make this read nicely.

AndresGuedez · 2018-09-10T21:42:24Z

The CircleCI mac failure is due to the lack of early connection close detection in macOS (see #4299). Per the conversation in that issue, I am disabling the failing test on macOS.

lizan · 2018-09-11T05:46:37Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // The delayed close timeout is for downstream connections managed by the HTTP connection manager.
+  // It is defined as a grace period after connection close processing has been locally initiated
+  // during which Envoy will flush the write buffers for the connection and await the peer to close
+  // the connection (i.e., issue a TCP FIN/RST).


In the case flush the write buffer and delay close, we should issue TCP FIN but not RST, the expected behavior is issue FIN after flush writing, and RST on timeout.

I've clarified this comment since this is referring to the client shutting down the connection and Envoy receiving the FIN/RST.

With regards to Envoy issuing FIN vs RST, the behavior is for a shutdown(_, SHUT_WR) to be issued when the flush write is successfully completed. If the delayed close timeout triggers, a close() is issued on the socket, which will trigger FIN or RST depending on the state of the socket/connection.

What benefit are we trying to get out of always issuing RST on timeout? It will likely lead to faster reclamation of resources which is nice, but I think Envoy's behavior regarding FIN vs RST should be applied uniformly across timeout/error conditions and therefore should be tracked as a new issue.

Signed-off-by: Andres Guedez <aguedez@google.com>

dnoe · 2018-09-11T17:32:23Z

source/common/network/connection_impl.cc

-    close_with_flush_ = true;
+    RELEASE_ASSERT(type == ConnectionCloseType::FlushWrite ||
+                       type == ConnectionCloseType::FlushWriteAndDelay,
+                   "");


The convention is generally to use ASSERT for this, if you feel strongly about RELEASE_ASSERT I'll defer to senior maintainer for the decision.

dnoe · 2018-09-12T00:49:34Z

.circleci/config.yml

     resource_class: xlarge
     working_directory: /source
     environment:
-       BAZEL_REMOTE_CACHE: https://storage.googleapis.com/envoy-circleci-bazel-cache/


Let's plan to revert these changes before this gets merged. If we do disable this for all of Envoy, I'd like to do it in a separate PR and consult with maintainers. I think you can leave it in for now with this comment in place so senior maintainer gets a heads up.

dnoe · 2018-09-12T01:01:02Z

test/common/network/connection_impl_test.cc

+        return new Buffer::WatermarkBuffer(below_low, above_high);
+      }));
+
+  auto timer = new Event::MockTimer(&dispatcher);


That explanation makes sense. I get nervous when I see bare new calls, especially without a comment. I think this test might be doing the opposite of the pattern I usually see, which is to have a unique_ptr here in the test body which is std::moved into the right place to be returned by Envoy::MockBufferFactory::create() but only after you steal a bare pointer out of the unique_ptr to use when setting up any the gmock expectations. This is a clear case of dangerous cheating the unique_ptr, but it's ok in this limited test case example since we know the unique pointer scope lives as long as the body of this test. I think it leaves the object life cycle a bit clearer defined than if you don't use unique_ptr here.

If this doesn't work in your case, please add a comment explaining the object lifecycle at the bare news. Happy to chat more about this in person tomorrow too if it helps.

Signed-off-by: Andres Guedez <aguedez@google.com>

dnoe

Nice, thanks. Tagging @zuercher for senior review.

AndresGuedez · 2018-09-12T03:08:36Z

@dnoe -- thanks for the review!

zuercher

Thanks for taking this on.

zuercher · 2018-09-12T16:31:37Z

include/envoy/network/connection.h

    // Counter* as this is an optional counter. Bind errors will not be tracked if this is nullptr.
    Stats::Counter* bind_errors_;
+    // Optional counter. Delayed close semantics are only used by HTTP connections.
+    Stats::Counter* delayed_close_timeouts_;


Let's remove the reference to HTTP, since this isn't necessarily HTTP-specific. I think the comment can just mirror the one for bind_errors_.

Agreed the delayed close timeout is not inherently HTTP specific, but the intent of the comment is to point out that as currently implemented it only affects HTTP connections, since the H{1,2} codecs are the only callers of setDelayedCloseTimeout() on a connection to enable it.

I'm fine removing this however if you don't think the note/clarification is required.

My feeling is that it's just a comment we'll have to delete (or will be incorrect) when someone inevitably enables this for a different purpose.

Ok. Cleaned up the comment.

zuercher · 2018-09-12T16:32:20Z

include/envoy/network/connection.h

+
+  /**
+   * Set the timeout for delayed connection close()s.
+   * This is only used for downstream connections processing HTTP/1 and HTTP/2.


Likewise, remove this reference (or at least the "only" qualifier).

Please document (probably on close) the relationship between this timeout value and the various close options. I had assumed that ConnectionCloseType::FlushWrite behavior was meant to be unchanged, but really it's going to wait up to the delayed close timeout, if set, to complete its flush before closing.

source/common/network/connection_impl.cc

zuercher · 2018-09-12T16:46:08Z

source/common/network/connection_impl.cc


+void ConnectionImpl::onDelayedCloseTimeout() {
+  ENVOY_CONN_LOG(debug, "triggered delayed close", *this);
+  ASSERT(connection_stats_->delayed_close_timeouts_);


Prefer that you test if delayed_close_timeouts_ is null (similar to how bind_errors_ optionality is handled). Or else, ASSERT the pointer is non-null when the delayed close timeout is configured.

zuercher · 2018-09-12T17:03:42Z

source/common/network/connection_impl.cc

+  if (delayed_close_timer_) {
+    // It's ok to disable even if the timer has already fired.
+    delayed_close_timer_->disableTimer();
+    delayed_close_timer_.reset();


Nit: don't need to reset -- it's about to be destroyed.

zuercher · 2018-09-12T17:51:42Z

test/common/network/connection_impl_test.cc

-  }
-};
+  } // namespace Network
+};  // namespace Envoy


Nit: undo these -- looks like fix format got confused at some point.

tools/check_format.py fails when this is reverted -- 'FixNamespaceComments' appears to be required.

I downloaded this file and it seems to pass format checks locally without the stray namespace comments. Has it failed CI's format check?

Reverted comments. I was looking at the wrong lines... Thanks.

zuercher · 2018-09-12T17:58:54Z

test/integration/http_integration.cc

 }

-void IntegrationCodecClient::waitForDisconnect() {
+bool IntegrationCodecClient::waitForDisconnect(uint32_t time_to_wait) {


Nit: use std::chrono::millisconds here to match the other integration tests.

zuercher · 2018-09-12T18:01:27Z

test/integration/http_integration.cc

+  if (wait_timer_triggered && !disconnected_) {
+    return false;
+  }
  EXPECT_TRUE(disconnected_);


Should disable the timer if it hasn't fired.

zuercher · 2018-09-12T18:05:15Z

test/integration/integration_test.cc

+
+  response->waitForEndStream();
+  // The delayed close timeout should trigger since client is not closing the connection.
+  EXPECT_TRUE(codec_client_->waitForDisconnect(750));


Have you given this the runs_per_test treatment?

Yeah, I've done extensive flake testing and did not hit any issues. I've adjusted the timeouts though to make it even more resistant since the CI environment is considerably more flake prone than my machine.

Signed-off-by: Andres Guedez <aguedez@google.com>

zuercher

Thanks. One small thing, but otherwise looks good to me.

zuercher · 2018-09-13T16:54:36Z

source/common/network/connection_impl.cc

    // Create and activate a timer which will immediately close the connection if triggered.
    // A config value of 0 disables the timeout.
    if (delayed_close_timeout_set) {
+      ASSERT(connection_stats_->delayed_close_timeouts_ != nullptr);


Sorry, I meant in setDelayedCloseTimeout.I don't want someone to enable this feature on a connection and then only later get an assertion about this stat.

connection_stats_ is set via setConnectionStats(), so asserting on the counter != nullptr in setDelayedCloseTimeout() will introduce an ordering dependency (which is not currently met). I think it would be preferable to keep these two functions independent of each other.

Unfortunately, initialization of ConnectionImpls is spread out between codec creation (allocation and call to setDelayedCloseTimeout) and ConnectionManagerImpl::initializeReadFilterCallbacks() where setConnectionStats() is called. Therefore, adding an initFinished() function where the ASSERT could be safely issued will require some work.

I can mark this with a TODO so we can move it into the appropriate place when such a function is added. What do you think?

Or just make setting connection_state_->delayed_close_timeouts_ optional? (E.g. don't assert, just don't increment if it's null?)

Signed-off-by: Andres Guedez <aguedez@google.com>

zuercher

Thanks!

AndresGuedez · 2018-09-14T13:24:39Z

@zuercher -- Thanks for reviewing!

AndresGuedez · 2018-09-17T17:26:44Z

This failed with a timeout in the SDS integration test. Can you check if your branch has c4211b3 (which was supposed to fix the timeout).

I don't have that commit in my branch; I'll merge and kick off new CI runs.

ggreenway · 2018-09-19T16:16:34Z

@mattklein123 Please take a look.

mattklein123

Very nice. Love seeing this implemented and the flush timeout leak fixed also. A few small questions/comments.

mattklein123 · 2018-09-21T18:02:17Z

source/common/http/http1/codec_impl.cc

+                                           Http1Settings settings,
+                                           std::chrono::milliseconds delayed_close_timeout)
+    : ConnectionImpl(connection, HTTP_REQUEST), callbacks_(callbacks), codec_settings_(settings) {
+  connection_.setDelayedCloseTimeout(delayed_close_timeout);


What's the reasoning for doing this set in the codec? It seems like something that is unrelated to the codec itself and should be done by higher layer code? Same question for the HTTP/2 codec below.

It's not related to the codec directly but this particular delayed_close_timeout is HTTP only since it is managed via the HCM configuration. This seemed to be the most appropriate place to set the timeout for HTTP connections just after they are constructed.

Since the timeout value is intended to be network filter specific, I wouldn't expect any layer higher than the corresponding filter to issue the call to setDelayedCloseTimeout().

I agree it should be done in the filter. Why not do it in the HCM filter and avoid codec modifications?

Agreed it's best to contain it within the HCM. I revisited the code and moved setting the timeout to the HCM config.

mattklein123 · 2018-09-21T18:04:35Z

source/common/network/connection_impl.cc

+
+    // No need to continue if a FlushWrite/FlushWriteAndDelay has already been issued and there is a
+    // pending delayed close.
+    if (delayed_close_) {


Does this happen in practice? Mostly just wondering about the flow that causes this. Perhaps flesh out the comment with some examples of cases where this happens?

Yes, I initially tried an ASSERT() which triggered in multiple tests during a //test/... run. I just tracked down a failure during an integration_test and added it to the comment. Based on that example and code inspection, this conditional can evaluate true under fairly standard use cases such as when Envoy initiates the close() sequence during codec processing.

mattklein123 · 2018-09-21T18:04:50Z

source/common/network/connection_impl.cc

+    }
+
+    delayed_close_ = true;
+    bool delayed_close_timeout_set = delayedCloseTimeout().count() > 0;


Signed-off-by: Andres Guedez <aguedez@google.com>

Instead, constrain setting the delayed close timeout to the HCM config, which owns it. Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123

Thanks, one small change and I think you also need a master merge.

mattklein123 · 2018-09-23T04:22:28Z

source/extensions/filters/network/http_connection_manager/config.cc

 HttpConnectionManagerConfig::createCodec(Network::Connection& connection,
                                         const Buffer::Instance& data,
                                         Http::ServerConnectionCallbacks& callbacks) {
+  connection.setDelayedCloseTimeout(delayed_close_timeout_);


I think I would recommend setting this in ConnectionManagerImpl::initializeReadFilterCallbacks. It's earlier than here and also near where we setup some other connection level stuff.

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123

LGTM. Can you merge master and we can 🚢? Thank you!

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123

LGTM, sorry, one more doc thing.

mattklein123 · 2018-09-24T20:14:04Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  //
+  // A value of 0 will completely disable delayed close processing, and the downstream connection's
+  // socket will be closed immediately after the write flush is completed.
+  google.protobuf.Duration delayed_close_timeout = 26 [(gogoproto.stdduration) = true];


Sorry I forget this from before. Can you add this to the release notes also with a link to this field?

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123

Awesome!

AndresGuedez · 2018-09-25T12:50:20Z

@mattklein123 Thanks for the review!

This reverts commit aa9478f. Signed-off-by: Jose Nino <jnino@lyft.com>

This PR reverts #4382. When deploying at Lyft we noticed crashes on here where we might be derefencing the connection_stats_ pointer after the point has been reset. Note: this PR keeps the changes to the API made in the original PR but tags the field as not implemented. This is what we have done in the past for reverts that involve changes that change the API. Signed-off-by: Jose Nino <jnino@lyft.com>

Re-enable the changes reverted in 9d32e5c, which were originally merged as part of envoyproxy#4382. Signed-off-by: Andres Guedez <aguedez@google.com>

Fixes a segfault introduced in envoyproxy#4382 due to a connection tear down race condition when the delayed close timer triggers after connection state has been reset via closeSocket(). Signed-off-by: Andres Guedez <aguedez@google.com>

Re-enable the changes reverted in 9d32e5c, which were originally merged as part of #4382. Signed-off-by: Andres Guedez <aguedez@google.com>

This PR reverts envoyproxy#4382. When deploying at Lyft we noticed crashes on here where we might be derefencing the connection_stats_ pointer after the point has been reset. Note: this PR keeps the changes to the API made in the original PR but tags the field as not implemented. This is what we have done in the past for reverts that involve changes that change the API. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

…ix (envoyproxy#4587) Re-enable the changes reverted in 9d32e5c, which were originally merged as part of envoyproxy#4382. Signed-off-by: Andres Guedez <aguedez@google.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

AndresGuedez added 2 commits September 6, 2018 12:29

Add HTTP/2 integration tests.

94a835a

HTTP/2 integration tests added for delayed close processing. Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez changed the title ~~Delayed conn close~~ network: delayed conn close Sep 10, 2018

AndresGuedez added 2 commits September 10, 2018 10:57

Kick CI

e2176aa

Signed-off-by: Andres Guedez <aguedez@google.com>

Adjust timeout values to prevent test flaking.

3a67ecb

Increase the timeout values for the common case (no timeout trigerred) tests to prevent test flakiness. Signed-off-by: Andres Guedez <aguedez@google.com>

zuercher assigned alyssawilk Sep 10, 2018

alyssawilk assigned dnoe and unassigned alyssawilk Sep 10, 2018

dnoe suggested changes Sep 10, 2018

View reviewed changes

lizan reviewed Sep 11, 2018

View reviewed changes

AndresGuedez added 3 commits September 11, 2018 10:07

Small cleanup and clarify comments.

30eb02e

Signed-off-by: Andres Guedez <aguedez@google.com>

Add clarifying comments to unit tests.

9ddf9d3

Signed-off-by: Andres Guedez <aguedez@google.com>

Disable bazel remote cache due to CI flaking.

8e716eb

Signed-off-by: Andres Guedez <aguedez@google.com>

dnoe reviewed Sep 12, 2018

View reviewed changes

Minor cleanup per style guidelines and best practices

e71f798

Signed-off-by: Andres Guedez <aguedez@google.com>

dnoe approved these changes Sep 12, 2018

View reviewed changes

dnoe previously approved these changes Sep 12, 2018

View reviewed changes

dnoe assigned zuercher Sep 12, 2018

zuercher reviewed Sep 12, 2018

View reviewed changes

Cleanup: comments and asserts/checks.

6130f28

Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez dismissed dnoe’s stale review via 6130f28 September 13, 2018 13:32

AndresGuedez added 3 commits September 13, 2018 09:53

Adjust integration test timeouts for flake resistance.

890471e

Signed-off-by: Andres Guedez <aguedez@google.com>

Revert namespace comments erronously introduced.

aae3b52

Signed-off-by: Andres Guedez <aguedez@google.com>

Kick CI

b8eb565

Signed-off-by: Andres Guedez <aguedez@google.com>

zuercher reviewed Sep 13, 2018

View reviewed changes

Change ASSERT to runtime check.

6071d31

Signed-off-by: Andres Guedez <aguedez@google.com>

zuercher previously approved these changes Sep 13, 2018

View reviewed changes

zuercher assigned ggreenway Sep 18, 2018

zuercher previously approved these changes Sep 19, 2018

View reviewed changes

ggreenway previously approved these changes Sep 19, 2018

View reviewed changes

ggreenway assigned mattklein123 Sep 19, 2018

mattklein123 requested changes Sep 21, 2018

View reviewed changes

Add clarifying comment and minor const correctness fix.

9d86332

Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez dismissed stale reviews from ggreenway and zuercher via 9d86332 September 21, 2018 20:50

Revert changes to http{1,2} codecs.

435a518

Instead, constrain setting the delayed close timeout to the HCM config, which owns it. Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123 requested changes Sep 23, 2018

View reviewed changes

Set the delayed close timeout for a connection in the HCM.

f2c81ed

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123 reviewed Sep 24, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into delayed-conn-close

6704243

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123 reviewed Sep 24, 2018

View reviewed changes

Update release notes with delayed close timeout.

31bcaaa

Signed-off-by: Andres Guedez <aguedez@google.com>

mattklein123 approved these changes Sep 24, 2018

View reviewed changes

mattklein123 merged commit aa9478f into envoyproxy:master Sep 24, 2018

junr03 pushed a commit that referenced this pull request Oct 1, 2018

Revert "network: delayed conn close (#4382)"

27fd24d

This reverts commit aa9478f. Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 mentioned this pull request Oct 1, 2018

Revert aa9478f06d613cd7b845e609a8c20c1ce116dad5 #4581

Merged

AndresGuedez mentioned this pull request Oct 2, 2018

Investigate crash introduced in #4382 #4583

Closed

AndresGuedez added a commit to AndresGuedez/envoy that referenced this pull request Oct 2, 2018

Revert 9d32e5c envoyproxy#4382

a6a2600

Re-enable the changes reverted in 9d32e5c, which were originally merged as part of envoyproxy#4382. Signed-off-by: Andres Guedez <aguedez@google.com>

AndresGuedez mentioned this pull request Oct 2, 2018

network: reintroduce #4382 (delayed conn close) with segv fix #4587

Merged

Conversation

AndresGuedez commented Sep 10, 2018

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndresGuedez commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

AndresGuedez commented Sep 12, 2018

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!