Fixes environment-dependent failures in MixerFaultTest#2144
Fixes environment-dependent failures in MixerFaultTest#2144duderino merged 1 commit intoistio:release-1.1from dmitri-d:fix-happy-path
Conversation
googlebot
added
the
cla: yes
|
@duderino: This should fix https://github.com/istio/proxy/issues/2124. |
test/integration/int_server.cc
Outdated
| close_callback_(close_callback) { | ||
| if (request_callback) { | ||
| request_callback_ = [this, &request_callback]( | ||
| instrumented_request_callback_ = [this]( |
There was a problem hiding this comment.
Ah! Super cool. I wonder if an equivalent fix would have been to capture the request_callback lambda by value?
There was a problem hiding this comment.
Didn't think of that, brain == fried. definitely worth a try!
There was a problem hiding this comment.
Great catch! If you fllow @duderino advice, could you try capture using expression? It might cost less while absolutely never more expensive. Something like
request_callback_ = [this, request_callback = std::move(request_callback) ](
|
@dmitri-d thank you so much for fixing my bug. I really appreciate it. Technically, I can't merge this until after 1.1 goes out. https://discuss.istio.io/t/istio-1-1-lockdown-release-status/1275 . Let me talk to the other admins and see if the no merge rule applies to test cases. Seems like it shouldn't but.... Also, if not merging this for a week or so has any adverse impact for you please let me know. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmitri-d, duderino The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@duderino this shouldn't impact us, we'll apply it locally until it gets merged. |
|
@duderino: updated to pass callbacks to lambdas by value as you suggested. |
|
/retest |
1 similar comment
|
/retest |
|
The fix has been merged into master, closing the PR here. |
|
It'd be nice to have this in 1.1 too for subsequent 1.1.x releases, so reopening it. Thanks @dmitri-d |
6 participants
What this PR does / why we need it:
Fixes intermittent/environment-dependent failures in MixerFaultTest. Please see https://github.com/istio/proxy/issues/2124.
Special notes for your reviewer:
The failures were due to lambda parameter in ServerCallbackHelper::ServerCallbackHelper going out of scope.