Added a test to verify vhds updates when client closes connection #11341
Conversation
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
| @@ -37,6 +37,8 @@ void OnDemandRouteUpdate::setDecoderFilterCallbacks(Http::StreamDecoderFilterCal | |||
| callbacks_ = &callbacks; | |||
| } | |||
|
|
|||
| void OnDemandRouteUpdate::onDestroy() { route_config_updated_callback_.reset(); } | |||
There was a problem hiding this comment.
Thanks for adding a test for this!
So you're resetting the callback, but what tells the caller of the callback to not call it?
you've created this lambda capturing this of the filter, and if the filter is destroyed, and the resolution completes, won't the callback still be called?
There was a problem hiding this comment.
The callback pointer is eventually stored in a weak_ptr, which is verified before the invocation: https://github.com/envoyproxy/envoy/blob/master/source/common/router/rds_impl.cc#L288. A longer description of what's going on in relation to the callback is here: #9784 (comment).
| createDeltaDiscoveryResponseWithResourceNameUsedAsAlias(); | ||
| vhds_stream_->sendGrpcMessage(vhds_update); | ||
|
|
||
| codec_client_->close(); |
There was a problem hiding this comment.
So I think a more interesting test would be to reset the stream, but not the connection. In that case the HCM will persist beyond the life of the stream, and I think you'll hit the lifetime issue Matt and I see.
I think rather than a simple lambda, you need something with a bit more state, so you can cancel the actual callback if the filter goes away.
There was a problem hiding this comment.
I think rather than a simple lambda, you need something with a bit more state, so you can cancel the actual callback if the filter goes away.
The state of the callback (whether it and the filter-chain are still active) is handled implicitly via shared_ptr - weak_ptr combination (pls. see #9784 (comment) for more details)
There was a problem hiding this comment.
If it's not clear in the code, (which IMO it's not, especially given how many autos there are in here:
https://github.com/envoyproxy/envoy/blob/master/source/common/router/rds_impl.cc#L288
making it hard to tell what's going on w.r.t. weak pointers - mind adding some comments to onDestroy to explain it for posterity?
Also, please change this test to reset the stream without closing the connection.
thanks!
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
|
| // EXPECT_EQ( | ||
| // initial_config_reloads + 1UL, | ||
| // TestUtility::findCounter(stats, "http.config_test.vhds.my_route.config_reload")->value()); | ||
| cleanupUpstreamAndDownstream(); |
There was a problem hiding this comment.
let's wait for the client_codec_->waitForEndStream() and assert the connection is connected, to make sure we're covering the case we're trying to cover (stream is reset,connection is not)
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
|
Please note that trying to do |
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
|
hm, coverage is still failing after I kicked it this morning. Could you try a master merge? |
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
|
|
Could you update the PR description to follow the template? As soon as that's done I am happy to merge. |
done |
This is to help with envoyproxy#9784 Risk Level: Low (added a single test) Testing: a new unit test and integration test Docs Changes: n/a Release Notes: n/a Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
This is to help with envoyproxy#9784 Risk Level: Low (added a single test) Testing: a new unit test and integration test Docs Changes: n/a Release Notes: n/a Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
This is to help with envoyproxy#9784 Risk Level: Low (added a single test) Testing: a new unit test and integration test Docs Changes: n/a Release Notes: n/a Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
This is to help with #9784
Risk Level: Low (added a single test)
Testing: a new unit test
Docs Changes: n/a
Release Notes: n/a