Skip to content

Restart LB call after client load report completion, if needed.#13417

Merged
markdroth merged 1 commit intogrpc:masterfrom
markdroth:grpclb_memory_leak_fix
Nov 16, 2017
Merged

Restart LB call after client load report completion, if needed.#13417
markdroth merged 1 commit intogrpc:masterfrom
markdroth:grpclb_memory_leak_fix

Conversation

@markdroth
Copy link
Copy Markdown
Member

Fixes #13350.

The problem here was that if the client load report completion callback was in flight when we received status, then we were not cleaning up. We were already handling this properly if we received status when the client load report timer had not yet fired (see send_client_load_report_locked()), but we were not handling the case where sending the load report itself was actually in flight. This PR makes the logic in client_load_report_done_locked() match that of send_client_load_report_locked().

Note that this is more than a simple memory leak; it could actually cause a client to fail to restart the LB call, thus continuing to use an increasingly stale serverlist.

@markdroth markdroth requested a review from dgquintas November 16, 2017 16:40
@markdroth markdroth requested a review from ctiller as a code owner November 16, 2017 16:40
@grpc-testing
Copy link
Copy Markdown

****************************************************************

libgrpc.so

     VM SIZE                                                                      FILE SIZE
 ++++++++++++++ GROWING                                                        ++++++++++++++
  +0.0%     +16 [None]                                                            +176  +0.0%
  +0.2%     +32 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc     +32  +0.2%
       +43%     +38 client_load_report_done_locked                                     +38   +43%

  +0.0%     +48 TOTAL                                                             +208  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]



@grpc-testing
Copy link
Copy Markdown

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

[microbenchmarks] No significant performance differences

@markdroth
Copy link
Copy Markdown
Member Author

Known issues: #12510 #13085 #12932 #13404 #12892 #13178 #13421

@markdroth markdroth merged commit 7fb7774 into grpc:master Nov 16, 2017
@markdroth markdroth deleted the grpclb_memory_leak_fix branch November 16, 2017 21:46
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants