Skip to content

Commit

Permalink
backport to v1.13: test flake: fix flake in ApiListener integration t…
Browse files Browse the repository at this point in the history
…est (#9808) (#13316)

Description: previously the test was not waiting for the expectation on the server's thread to complete. Therefore, there was a use after free race condition with the ApiListener's TlsCachingDateProvider. This PR makes it so that the test waits for the expectation to be fulfilled  and thus prevents the race.
Risk Level: low
Testing: ran integration test a few thousand times locally on a linux machine.

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Antonio Vicente <avd@google.com>
  • Loading branch information
antoniovicente authored Sep 29, 2020
1 parent 4e32599 commit e744e5b
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 7 deletions.
2 changes: 1 addition & 1 deletion VERSION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.13.5
1.13.6
6 changes: 6 additions & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
Version history
---------------

1.13.6 (September 29, 2020)
===========================
Changes
-------
* test: fix flaky test.

1.13.5 (September 29, 2020)
===========================
Changes
Expand Down
13 changes: 7 additions & 6 deletions test/integration/api_listener_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "test/test_common/environment.h"
#include "test/test_common/utility.h"

#include "absl/synchronization/notification.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"

Expand Down Expand Up @@ -76,13 +77,15 @@ name: api_listener
NiceMock<Http::MockStreamEncoder> stream_encoder_;
};

ACTION_P(Notify, notification) { notification->Notify(); }

INSTANTIATE_TEST_SUITE_P(IpVersions, ApiListenerIntegrationTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);

TEST_P(ApiListenerIntegrationTest, Basic) {
ConditionalInitializer test_ran;
test_server_->server().dispatcher().post([this, &test_ran]() -> void {
absl::Notification done;
test_server_->server().dispatcher().post([this, &done]() -> void {
ASSERT_TRUE(test_server_->server().listenerManager().apiListener().has_value());
ASSERT_EQ("api_listener", test_server_->server().listenerManager().apiListener()->get().name());
ASSERT_TRUE(test_server_->server().listenerManager().apiListener()->get().http().has_value());
Expand All @@ -97,17 +100,15 @@ TEST_P(ApiListenerIntegrationTest, Basic) {
Http::TestHeaderMapImpl expected_response_headers{{":status", "200"}};
EXPECT_CALL(stream_encoder_, encodeHeaders(_, false));
EXPECT_CALL(stream_encoder_, encodeData(_, false));
EXPECT_CALL(stream_encoder_, encodeData(BufferStringEqual(""), true));
EXPECT_CALL(stream_encoder_, encodeData(BufferStringEqual(""), true)).WillOnce(Notify(&done));

// Send a headers-only request
stream_decoder.decodeHeaders(
Http::HeaderMapPtr(new Http::TestHeaderMapImpl{
{":method", "GET"}, {":path", "/api"}, {":scheme", "http"}, {":authority", "host"}}),
true);

test_ran.setReady();
});
test_ran.waitReady();
ASSERT_TRUE(done.WaitForNotificationWithTimeout(absl::Seconds(1)));
}

} // namespace
Expand Down

0 comments on commit e744e5b

Please sign in to comment.