Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

grpc: Clean up initialization and allocation of timers in the test #4226

Merged
merged 2 commits into from
Aug 23, 2018

Conversation

jmarantz
Copy link
Contributor

Signed-off-by: Joshua Marantz jmarantz@google.com

Description: The management of the timer_ variable in the test class was a little funky, and looked leak-prone. This PR cleans this up a little,. Without this PR, #4180 memory-leaks on this test, I think due to overwriting timer_. This is all toward resolution of #4160

Risk Level: Low.
Testing: //test/common/config:grpc_mux_impl_test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

@htuch ptal; this should be easy.

@htuch htuch self-assigned this Aug 22, 2018
@@ -76,8 +70,8 @@ class GrpcMuxImplTest : public testing::Test {
NiceMock<Event::MockDispatcher> dispatcher_;
Runtime::MockRandomGenerator random_;
Grpc::MockAsyncClient* async_client_;
Event::MockTimer* timer_;
Event::TimerCb timer_cb_;
Event::MockTimer* timer_; // Only used by ResetStream
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a nice cleanup. Can you just move these two variables to be local to ResetStream? This will make reasoning even simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

Anything else needed before you merge?

@htuch htuch merged commit 78ad2ef into envoyproxy:master Aug 23, 2018
@jmarantz jmarantz deleted the timer-allocation-cleanup branch August 23, 2018 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants