+20%     +59 pf_shutdown_locked                                                                       +59   +20%
       +14%     +17 [Unmapped]                                                                               +17   +14%
      +0.5%      +9 pf_update_locked                                                                          +9  +0.5%
      +6.3%      +8 pf_pick_locked                                                                            +8  +6.3%

 -------------- SHRINKING                                                                            --------------
  -0.2%     -16 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                 -16  -0.2%
      -8.3%    -157 rr_connectivity_changed_locked                                                          -157  -8.3%
      [DEL]     -57 rr_subchannel_list_ref(rr_subchannel_list*, char const*) [clone .part.0]                 -57  [DEL]
      -0.8%      -1 [Unmapped]                                                                                -1  -0.8%

  +0.1% +1.53Ki TOTAL                                                                                +10.1Ki  +0.2%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

Reviewed 5 of 5 files at r10.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

src/core/ext/filters/client_channel/lb_policy.cc, line 181 at r10 (raw file):

    policy->request_reresolution = NULL;
    if (GRPC_TRACER_ON(grpc_lb_trace) && error == GRPC_ERROR_NONE) {
      gpr_log(GPR_DEBUG, "LB policy %p: reresolution requested.", policy);

It might be a good idea to log this even in the cancellation case.  The message can be made a bit more generic, so that it works for both cases:
gpr_log(GPR_DEBUG, "LB policy %p: scheduling re-resolution closure with error=%s", policy,
        grpc_error_string(error));

src/core/ext/filters/client_channel/lb_policy.cc, line 181 at r10 (raw file):

    policy->request_reresolution = NULL;
    if (GRPC_TRACER_ON(grpc_lb_trace) && error == GRPC_ERROR_NONE) {
      gpr_log(GPR_DEBUG, "LB policy %p: reresolution requested.", policy);

Instead of "LB policy", let's use the value of grpc_lb_trace.name.
Same thing on line 185.

src/core/ext/filters/client_channel/lb_policy.h, line 227 at r10 (raw file):

void grpc_lb_policy_try_reresolve(grpc_exec_ctx *exec_ctx,
                                  grpc_lb_policy *policy,
                                  grpc_tracer_flag grpc_lb_trace,

The tracer can be passed in as a pointer.

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 1886 at r8 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
I guess args->lb_policy != chand->lb_policy can't cover all the cases that the LB policy is shutting down, although the counterexample is quite rare. Still taking cc_destroy_channel_elem() as an example, it doesn't assign chand->lb_policy a new value after unrefing chand->lb_policy. So if the closure is scheduled for the latest LB policy, args->lb_policy != chand->lb_policy is false. I think setting chand->lb_policy = NULL may help, but it looks like we don't have a convention to set the pointers to NULL in destructor.
Should I care about this special case?

cc_destroy_channel_elem() is not called until all refs to the channel stack have been released.  That won't happen until after shutdown is completed.  And the channel stack itself (including the channel_data struct) are freed after it runs, which is why it doesn't bother setting lb_policy to NULL after it unrefs it.
I don't believe that there are actually any cases where chand->lb_policy is not reset when the LB policy is shut down.

Comments from Reviewable

[trickle] No significant performance differences

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

src/core/ext/filters/client_channel/lb_policy.cc, line 181 at r10 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
It might be a good idea to log this even in the cancellation case.  The message can be made a bit more generic, so that it works for both cases:
gpr_log(GPR_DEBUG, "LB policy %p: scheduling re-resolution closure with error=%s", policy,
        grpc_error_string(error));

Done.

src/core/ext/filters/client_channel/lb_policy.cc, line 181 at r10 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Instead of "LB policy", let's use the value of grpc_lb_trace.name.
Same thing on line 185.

Done. This is a much better way to log! I feel we should use it anywhere we log instead of hardcoding RR or pick first for each log.

src/core/ext/filters/client_channel/lb_policy.h, line 227 at r10 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
The tracer can be passed in as a pointer.

Done.

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 1886 at r8 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
cc_destroy_channel_elem() is not called until all refs to the channel stack have been released.  That won't happen until after shutdown is completed.  And the channel stack itself (including the channel_data struct) are freed after it runs, which is why it doesn't bother setting lb_policy to NULL after it unrefs it.
I don't believe that there are actually any cases where chand->lb_policy is not reset when the LB policy is shut down.

Done.
Thanks for explaining. That is reassuring.

  I am just waiting for David to feel well enough to review #12878 before merging it, and then you can merge it into this branch to see if it fixes the asan problem you're seeing.

Reviewed 5 of 5 files at r11.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

src/core/ext/filters/client_channel/lb_policy.cc, line 181 at r10 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
Done. This is a much better way to log! I feel we should use it anywhere we log instead of hardcoding RR or pick first for each log.

There's no reason not to hard-code the value in pick_first.cc or round_robin.cc, since those files are specifically for the respective LB policies.  However, this approach is useful in cases where the two policies share common code (such as this and the subchannel_list.cc file in #12878).

Comments from Reviewable

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

src/core/ext/filters/client_channel/lb_policy.cc, line 181 at r10 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
There's no reason not to hard-code the value in pick_first.cc or round_robin.cc, since those files are specifically for the respective LB policies.  However, this approach is useful in cases where the two policies share common code (such as this and the subchannel_list.cc file in #12878).

I think there may be some small benefits not to hard-code. For example, we can avoid typos. Also, it may simplify some refactoring work like #12878.
That being said, I'm fine with the current code and don't feel urgent to change it :)

Comments from Reviewable

#12878 has been merged, so you can merge from master and hopefully move forward with this.  Thanks!

I have resolved the conflicts and I can compile it. But the tests can't be compiled on my laptop.. I also can't ssh to my workstation from China..   Just a couple of comments left.
I'm looking forward to having this merged!

Reviewed 6 of 6 files at r14.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 74 at r14 (raw file):

}

static void shutdown_locked(grpc_exec_ctx* exec_ctx, pick_first_lb_policy* p,

This code can probably be moved directly back into pf_shutdown_locked() now, since we're no longer calling it from anywhere else.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 539 at r14 (raw file):

        grpc_lb_subchannel_data_unref_subchannel(exec_ctx, sd,
                                                 "pf_candidate_shutdown");
        if (sd->subchannel_list == p->subchannel_list) {

We do not want to set the state to TRANSIENT_FAILURE unless all subchannels have failed.  Instead, I think we should do this where we request reresolution (line 556 below), since that's already happening under the right conditions.

Comments from Reviewable

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +693 [None]                                                                               +4.22Ki  +0.1%
      +0.1%    +669 [Unmapped]                                                                           +4.19Ki  +0.1%
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.4%    +208 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                          +208  +1.4%
      [NEW] +1.57Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.6]                   +1.57Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.8]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +6.0%     +35 glb_shutdown_locked                                                                      +35  +6.0%
      +1.6%      +6 [Unmapped]                                                                                +6  +1.6%
   +43%    +187 src/core/ext/filters/client_channel/lb_policy.cc                                        +187   +43%
      [NEW]    +161 grpc_lb_policy_try_reresolve                                                            +161  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +1.3%    +176 src/core/ext/filters/client_channel/client_channel.cc                                   +176  +1.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
      +3.7%     +96 on_resolver_result_changed_locked                                                        +96  +3.7%
      +1.2%      +3 [Unmapped]                                                                                +3  +1.2%
  +1.7%     +80 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +80  +1.7%
      +238%    +298 pf_shutdown_locked                                                                      +298  +238%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]

 -------------- SHRINKING                                                                            --------------
  -3.4%    -192 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -192  -3.4%
     -17.0%    -345 rr_connectivity_changed_locked                                                          -345 -17.0%
      [DEL]    -260 shutdown_locked                                                                         -260  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.1]            -102  [DEL]

  +0.1% +1.12Ki TOTAL                                                                                +4.66Ki  +0.1%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 74 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
This code can probably be moved directly back into pf_shutdown_locked() now, since we're no longer calling it from anywhere else.

Done.
The counterpart in round_robin is also changed.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 539 at r14 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
We do not want to set the state to TRANSIENT_FAILURE unless all subchannels have failed.  Instead, I think we should do this where we request reresolution (line 556 below), since that's already happening under the right conditions.

Done.
I'm afraid this will be more different from the previous behavior. Previously, we shut down if all the subchannels are dead, and transition to TRANSIENT_FAILURE if there are still candidates. Now, we transition to TRANSIENT_FAILURE if all the subchannels are dead, and keep the old state if there are still candidates.
To me, it's more reasonable to transition to TRANSIENT_FAILURE whenever the checking subchannel is dead. Because that is a real failure and it's also transient (because we can either check the next one or re-resolve).

Comments from Reviewable

Corrupt JSON data (indicates timeout or crash):
    bm_fullstack_streaming_pump.BM_PumpStreamClientToServer_UDS__1.opt.new: 1
    bm_fullstack_streaming_pump.BM_PumpStreamClientToServer_UDS__64.counters.new: 1

[microbenchmarks] Performance differences noted:
Benchmark                                                             cpu_time    real_time
--------------------------------------------------------------------  ----------  -----------
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/262144  -4%         -4%

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +693 [None]                                                                               +4.21Ki  +0.1%
      +0.1%    +669 [Unmapped]                                                                           +4.18Ki  +0.1%
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.4%    +208 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                          +208  +1.4%
      [NEW] +1.57Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.6]                   +1.57Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.8]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +6.0%     +35 glb_shutdown_locked                                                                      +35  +6.0%
      +2.2%      +8 [Unmapped]                                                                                +8  +2.2%
   +43%    +187 src/core/ext/filters/client_channel/lb_policy.cc                                        +187   +43%
      [NEW]    +161 grpc_lb_policy_try_reresolve                                                            +161  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +1.3%    +176 src/core/ext/filters/client_channel/client_channel.cc                                   +176  +1.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
      +3.7%     +96 on_resolver_result_changed_locked                                                        +96  +3.7%
      +1.2%      +3 [Unmapped]                                                                                +3  +1.2%
  +1.7%     +80 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +80  +1.7%
      +238%    +298 pf_shutdown_locked                                                                      +298  +238%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]

 -------------- SHRINKING                                                                            --------------
  -3.4%    -192 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -192  -3.4%
     -17.0%    -345 rr_connectivity_changed_locked                                                          -345 -17.0%
      [DEL]    -260 shutdown_locked                                                                         -260  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.1]            -102  [DEL]

  +0.1% +1.12Ki TOTAL                                                                                +4.66Ki  +0.1%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +693 [None]                                                                               +4.21Ki  +0.1%
      +0.1%    +669 [Unmapped]                                                                           +4.18Ki  +0.1%
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.4%    +208 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                          +208  +1.4%
      [NEW] +1.57Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.6]                   +1.57Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.8]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +6.0%     +35 glb_shutdown_locked                                                                      +35  +6.0%
      +2.2%      +8 [Unmapped]                                                                                +8  +2.2%
   +43%    +187 src/core/ext/filters/client_channel/lb_policy.cc                                        +187   +43%
      [NEW]    +161 grpc_lb_policy_try_reresolve                                                            +161  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +1.3%    +176 src/core/ext/filters/client_channel/client_channel.cc                                   +176  +1.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
      +3.7%     +96 on_resolver_result_changed_locked                                                        +96  +3.7%
      +1.2%      +3 [Unmapped]                                                                                +3  +1.2%
  +1.7%     +80 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +80  +1.7%
      +238%    +298 pf_shutdown_locked                                                                      +298  +238%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]

 -------------- SHRINKING                                                                            --------------
  -3.4%    -192 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -192  -3.4%
     -17.0%    -345 rr_connectivity_changed_locked                                                          -345 -17.0%
      [DEL]    -260 shutdown_locked                                                                         -260  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.1]            -102  [DEL]

  +0.1% +1.12Ki TOTAL                                                                                +4.66Ki  +0.1%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

Corrupt JSON data (indicates timeout or crash):
    bm_fullstack_streaming_pump.BM_PumpStreamClientToServer_UDS__4096.counters.new: 1
    bm_fullstack_unary_ping_pong.BM_UnaryPingPong_InProcessCHTTP2_NoOpMutator_NoOpMutator__4096_0.counters.old: 1

[microbenchmarks] No significant performance differences

Just a couple of minor nits, and then we can get this merged!

Reviewed 2 of 2 files at r15, 1 of 1 files at r17.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 539 at r14 (raw file):
Previously, AspirinSJL (Juanli Shen) wrote…
Done.
I'm afraid this will be more different from the previous behavior. Previously, we shut down if all the subchannels are dead, and transition to TRANSIENT_FAILURE if there are still candidates. Now, we transition to TRANSIENT_FAILURE if all the subchannels are dead, and keep the old state if there are still candidates.
To me, it's more reasonable to transition to TRANSIENT_FAILURE whenever the checking subchannel is dead. Because that is a real failure and it's also transient (because we can either check the next one or re-resolve).

We definitely do not want to transition to TRANSIENT_FAILURE if we still have subchannels that we can try to connect to.  From the application's perspective, we are still trying to connect, so we should stay in state CONNECTING.  The fact that we have multiple subchannels and are failing over from one to the other should be hidden from the application.
Yes, it looks like this is a behavior change, but I think it's the right one.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 86 at r17 (raw file):

    GRPC_CLOSURE_SCHED(
        exec_ctx, pp->on_complete,
        GRPC_ERROR_CREATE_FROM_STATIC_STRING("Channel shutdown"));

Instead of creating a new error for each pending pick and again for the call to grpc_connectivity_state_set() below, let's create the error once above and ref it for each of these calls.

src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 183 at r17 (raw file):

    GRPC_CLOSURE_SCHED(
        exec_ctx, pp->on_complete,
        GRPC_ERROR_CREATE_FROM_STATIC_STRING("Channel shutdown"));

Same comment as in pick_first.cc: Let's create the error once and ref it for each call.

Instead of creating a new error for each pending pick and again for the call to grpc_connectivity_state_set() below, let's create the error once above and ref it for each of these calls.

Done.
Actually I deliberately created a new error for each use because glb_shutdown_locked() did so. To be consistent, I have also changed glb_shutdown_locked() to reuse the error.

src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc, line 183 at r17 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
Same comment as in pick_first.cc: Let's create the error once and ref it for each call.

Done.

Comments from Reviewable

[trickle] No significant performance differences

[microbenchmarks] Performance differences noted:
Benchmark                                                                                locks_per_iteration    nows_per_iteration
-------------------------------------------------------------------------------------------------  ---------------------  --------------------
BM_StreamingPingPong<InProcess, NoOpMutator, NoOpMutator>/134217728/2                              +31%
BM_StreamingPingPong<InProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/2                        +9%
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/134217728/2                           +31%
BM_StreamingPingPong<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/2                     +7%
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/134217728/2/0           +32%
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/134217728/2/1           +34%
BM_StreamingPingPongWithCoalescingApi<InProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/2/0     +12%
BM_StreamingPingPongWithCoalescingApi<InProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/2/1     +12%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/134217728/2/0        +14%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/134217728/2/1        +34%
BM_StreamingPingPongWithCoalescingApi<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/2/0  +11%
BM_StreamingPingPongWithCoalescingApi<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/2/1  +12%
BM_UnaryPingPong<InProcess, NoOpMutator, NoOpMutator>/134217728/134217728                          +40%
BM_UnaryPingPong<InProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/0                                                   +11%
BM_UnaryPingPong<InProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/134217728                    +16%
BM_UnaryPingPong<MinInProcess, NoOpMutator, NoOpMutator>/134217728/134217728                       +40%
BM_UnaryPingPong<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/0                         +8%                    +10%
BM_UnaryPingPong<MinInProcessCHTTP2, NoOpMutator, NoOpMutator>/134217728/134217728                 +13%

Looks great!  Thanks for doing this!

Reviewed 3 of 3 files at r18.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc, line 1740 at r18 (raw file):

  glb_policy->fallback_backend_addresses =
      extract_backend_addresses_locked(exec_ctx, addresses);
  if (glb_policy->lb_fallback_timeout_ms > 0 && glb_policy->rr_policy != NULL) {

If this PR gets merged after #13339, please make sure that we use this version of this line instead of the version in that PR.

Comments from Reviewable

I've just merged #13339

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +613 [None]                                                                               +4.58Ki  +0.1%
      +0.1%    +589 [Unmapped]                                                                           +4.55Ki  +0.1%
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
   +43%    +187 src/core/ext/filters/client_channel/lb_policy.cc                                        +187   +43%
      [NEW]    +161 grpc_lb_policy_try_reresolve                                                            +161  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +1.3%    +176 src/core/ext/filters/client_channel/client_channel.cc                                   +176  +1.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
      +3.7%     +96 on_resolver_result_changed_locked                                                        +96  +3.7%
      +1.2%      +3 [Unmapped]                                                                                +3  +1.2%
  +0.7%     +96 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                           +96  +0.7%
      [NEW] +1.59Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.6]                   +1.59Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.8]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +0.5%      +2 [Unmapped]                                                                                +2  +0.5%
  +0.4%     +16 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +16  +0.4%
      +208%    +260 pf_shutdown_locked                                                                      +260  +208%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]
      +0.8%      +1 [Unmapped]                                                                                +1  +0.8%

 -------------- SHRINKING                                                                            --------------
  -3.9%    -224 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -224  -3.9%
     -17.0%    -345 rr_connectivity_changed_locked                                                          -345 -17.0%
      [DEL]    -260 shutdown_locked                                                                         -260  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.1]            -102  [DEL]

  +0.1%    +864 TOTAL                                                                                +4.82Ki  +0.1%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

Corrupt JSON data (indicates timeout or crash):
    bm_fullstack_streaming_pump.BM_PumpStreamClientToServer_UDS__4096.opt.new: 1

[microbenchmarks] Performance differences noted:
Benchmark                                      cpu_time    real_time
---------------------------------------------  ----------  -----------
BM_PollAddFd                                   -9%         -9%
BM_PumpStreamServerToClient<InProcess>/262144  +4%         +4%

This PR has a memory leak bug that I don't understand. Looks like after removing nodes the AVL tree create may create new nodes, which are leaked.
@ctiller has any idea about this?
Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x4c15ab in __interceptor_malloc /home/development/llvm/3.8.1/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x80f077 in gpr_malloc /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:56:7
    #2 0x91a407 in new_node(void*, void*, gpr_avl_node*, gpr_avl_node*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:81:39
    #3 0x91df56 in rebalance(gpr_avl_vtable const*, void*, void*, gpr_avl_node*, gpr_avl_node*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:196:32
    #4 0x91d1e7 in remove_key(gpr_avl_vtable const*, gpr_avl_node*, void*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:276:12
    #5 0x91c30f in gpr_avl_remove /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:285:14
    #6 0x8e9397 in grpc_subchannel_index_unregister /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel_index.cc:235:9
    #7 0x8dec49 in disconnect(grpc_exec_ctx*, grpc_subchannel*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel.cc:246:3
    #8 0x8dea93 in grpc_subchannel_unref /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel.cc:268:5
    #9 0xa3af11 in grpc_lb_subchannel_data_unref_subchannel /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc:44:5
    #10 0xa44353 in rr_connectivity_changed_locked(grpc_exec_ctx*, void*, grpc_error*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc:447:5
    #11 0x6b49f3 in grpc_combiner_continue_exec_ctx /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/combiner.cc:261:5
    #12 0x6c5be0 in grpc_exec_ctx_flush /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/exec_ctx.cc:96:17
    #13 0x73b3e9 in end_worker(grpc_exec_ctx*, grpc_pollset*, grpc_pollset_worker*, grpc_pollset_worker**) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_epoll1_linux.cc:880:9
    #14 0x7314d6 in pollset_work(grpc_exec_ctx*, grpc_pollset*, grpc_pollset_worker**, long) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_epoll1_linux.cc:984:3
    #15 0x6c464c in grpc_pollset_work /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_posix.cc:238:10
    #16 0x8793fd in cq_next(grpc_completion_queue*, gpr_timespec, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/surface/completion_queue.cc:964:23
    #17 0x875190 in grpc_completion_queue_next /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/surface/completion_queue.cc:1045:10
    #18 0x7a2b35 in grpc::CompletionQueue::AsyncNextInternal(void**, bool*, gpr_timespec) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/cpp/common/completion_queue_cc.cc:56:15
    #19 0x7dd847 in grpc::CompletionQueue::NextStatus grpc::CompletionQueue::AsyncNext<gpr_timespec>(void**, bool*, gpr_timespec const&) /usr/local/google/home/juanlishen/wip/reresolution/grpc/include/grpc++/impl/codegen/completion_queue.h:157:12
    #20 0x7e0f27 in grpc::Server::SyncRequestThreadManager::PollForWork(void**, bool*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/cpp/server/server_cc.cc:276:13
    #21 0x7ff88c in grpc::ThreadManager::MainWorkLoop() /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/cpp/thread_manager/thread_manager.cc:122:30
    #22 0x7ff665 in grpc::ThreadManager::WorkerThread::Run() /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/cpp/thread_manager/thread_manager.cc:38:3
    #23 0x806bce in void std::_Mem_fn<void (grpc::ThreadManager::WorkerThread::*)()>::operator()<, void>(grpc::ThreadManager::WorkerThread*) const /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:601:11
    #24 0x8069f4 in void std::_Bind_simple<std::_Mem_fn<void (grpc::ThreadManager::WorkerThread::*)()> (grpc::ThreadManager::WorkerThread*)>::_M_invoke<0ul>(std::_Index_tuple<0ul>) /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:1731:18
    #25 0x80694f in std::_Bind_simple<std::_Mem_fn<void (grpc::ThreadManager::WorkerThread::*)()> (grpc::ThreadManager::WorkerThread*)>::operator()() /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/functional:1720:16
    #26 0x8062f6 in std::thread::_Impl<std::_Bind_simple<std::_Mem_fn<void (grpc::ThreadManager::WorkerThread::*)()> (grpc::ThreadManager::WorkerThread*)> >::_M_run() /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/thread:115:13
    #27 0x7ff604003a5f in execute_native_thread_routine /build/gcc-4.8-mW1ufQ/gcc-4.8-4.8.4/build/x86_64-linux-gnu/libstdc++-v3/src/c++11/../../../../../src/libstdc++-v3/src/c++11/thread.cc:84

Objects leaked above:
0x6040000b1c50 (48 bytes)

This leak also happens to the node which is created when the subchannel is created. So I guess I forgot to unref the subchannel somewhere.
Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x4c15ab in __interceptor_malloc /home/development/llvm/3.8.1/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x80f077 in gpr_malloc /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:56:7
    #2 0x91a407 in new_node(void*, void*, gpr_avl_node*, gpr_avl_node*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:81:39
    #3 0x91ed95 in rotate_left(gpr_avl_vtable const*, void*, void*, gpr_avl_node*, gpr_avl_node*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:128:30
    #4 0x91df0d in rebalance(gpr_avl_vtable const*, void*, void*, gpr_avl_node*, gpr_avl_node*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:193:13
    #5 0x91bddd in add_key(gpr_avl_vtable const*, gpr_avl_node*, void*, void*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:215:12
    #6 0x91b5c3 in gpr_avl_add /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:224:14
    #7 0x8e868c in grpc_subchannel_index_register /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel_index.cc:187:25
    #8 0x8e1817 in grpc_subchannel_create /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel.cc:376:10
    #9 0x8ed4b6 in client_channel_factory_create_subchannel(grpc_exec_ctx*, grpc_client_channel_factory*, grpc_subchannel_args const*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/transport/chttp2/client/insecure/channel_create.cc:43:24
    #10 0x8d95d2 in grpc_client_channel_factory_create_subchannel /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/client_channel_factory.cc:34:10
    #11 0xa3c715 in grpc_lb_subchannel_list_create /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc:126:35
    #12 0xa3f22e in rr_update_locked(grpc_exec_ctx*, grpc_lb_policy*, grpc_lb_policy_args const*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc:575:46
    #13 0xa3eb6e in round_robin_create(grpc_exec_ctx*, grpc_lb_policy_factory*, grpc_lb_policy_args*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc:659:3
    #14 0x718d42 in grpc_lb_policy_factory_create_lb_policy /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy_factory.cc:168:10
    #15 0x8db54a in grpc_lb_policy_create /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy_registry.cc:68:7
    #16 0xa1cb17 in create_rr_locked(grpc_exec_ctx*, glb_lb_policy*, grpc_lb_policy_args*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc:757:7
    #17 0xa1c05b in rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc:849:5
    #18 0xa23b16 in lb_on_response_received_locked(grpc_exec_ctx*, void*, grpc_error*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc:1665:13
    #19 0x6b49f3 in grpc_combiner_continue_exec_ctx /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/combiner.cc:261:5
    #20 0x6c5be0 in grpc_exec_ctx_flush /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/exec_ctx.cc:96:17
    #21 0x7670b1 in pollset_work(grpc_exec_ctx*, grpc_pollset*, grpc_pollset_worker**, long) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_poll_posix.cc:1057:22
    #22 0x6c464c in grpc_pollset_work /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_posix.cc:238:10
    #23 0x87ce97 in cq_pluck(grpc_completion_queue*, void*, gpr_timespec, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/surface/completion_queue.cc:1198:23
    #24 0x8755cb in grpc_completion_queue_pluck /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/surface/completion_queue.cc:1228:10
    #25 0x7a3c81 in grpc::CoreCodegen::grpc_completion_queue_pluck(grpc_completion_queue*, void*, gpr_timespec, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/cpp/common/core_codegen.cc:72:10
    #26 0x543dcd in grpc::CompletionQueue::Pluck(grpc::internal::CompletionQueueTag*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/include/grpc++/impl/codegen/completion_queue.h:253:15
    #27 0x64bcc5 in grpc::internal::BlockingUnaryCallImpl<grpc::testing::EchoRequest, grpc::testing::EchoResponse>::BlockingUnaryCallImpl(grpc::ChannelInterface*, grpc::internal::RpcMethod const&, grpc::ClientContext*, grpc::testing::EchoRequest const&, grpc::testing::EchoResponse*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/include/grpc++/impl/codegen/client_unary_call.h:71:9
    #28 0x64602a in grpc::Status grpc::internal::BlockingUnaryCall<grpc::testing::EchoRequest, grpc::testing::EchoResponse>(grpc::ChannelInterface*, grpc::internal::RpcMethod const&, grpc::ClientContext*, grpc::testing::EchoRequest const&, grpc::testing::EchoResponse*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/include/grpc++/impl/codegen/client_unary_call.h:41:10
    #29 0x640677 in grpc::testing::EchoTestService::Stub::Echo(grpc::ClientContext*, grpc::testing::EchoRequest const&, grpc::testing::EchoResponse*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/gens/src/proto/grpc/testing/echo.grpc.pb.cc:41:10

Objects leaked above:
0x604000001150 (48 bytes)

OK, the bug lies in the closure passing on. The closure is somehow lost so the LB policy has nothing to cancel during shutdown. I still can't understand how the closure can be lost. Will investigate tomorrow.

Do you mean that an scheduled closure isn't getting called prior to shutdown? That's surprising, as many other things would break if that were the case.

Which closure are you referring to?

I should have been clearer. The re-resolution closure is given back to the LB policy by the client channel at the end of callback if it's scheduled with GRPC_ERROR_NONE. When the LB policy is shutting down, it will schedule the re-resolution closure with GRPC_ERROR_CANCELLED to free the resource. Each closure should be scheduled with GRPC_ERROR_CANCELLED at the end to avoid leak. But if I add an assertion to the LB policy shutdown() function to assert that it must have a re-resolution closure to cancel, it crashes. So by "lost", I mean the closure is not given back to the LB policy at some point.
I am thinking that maybe when the LB policy is shutting down, the client channel is still working on re-resolution and hasn't given back the closure in time. But that shouldn't happen, because when we giving back the closure to the LB policy, we assert that the LB policy is not shutting down.
So I am confused why this is happening.

I'm wrong. The closure doesn't need to be passed back to the LB policies to be destroyed. It can be freed by the client channel directly if it's shutting down. Now I think the shutdown timing may have some bug.

Logically, I feel there may be a circular dependency. The LB policy can't shut down because it's waiting for channel_data to be destroyed and unref the LB policy. The channel_data can't be destroyed because it's waiting for the re-resolution closure to call GRPC_CHANNEL_STACK_UNREF(exec_ctx, chand->owning_stack, "re-resolution");. The re-resolution closure can't be scheduled because it's waiting for the LB policy to shut down.
So I run GRPC_VERBOSITY=debug GRPC_TRACE=stream_refcount,lb_policy_refcount tools/run_tests/run_tests.py -l c++ -r "grpclb_end" -c asan -S to check the refcounts. But the log doesn't make much sense to me. I have some sample runs at the end of this comment.
I think the fix is to cancel the re-resolution closure as soon as we know the client channel will shut down. I tried to cancel the closure wherever the resolver is set to NULL, but it only decreases the flakiness to ~0.01 instead of eliminating it.
=============================================
The log below is most frequent. The refcounting shows that only client channel 0x616000006cf8:0x616000006c80 is leaked, LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:20872 fd=18 0x6100000065e0 shows that a balancer server is leaked, and the stacktrace shows that an AVL tree node is directly leaked.
D1116 19:42:03.586524853  184302 lb_policy.cc:72]            LB_POLICY: 0x60b0000068c0   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 19:42:03.586544973  184302 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 2->1 watch_lb_policy
D1116 19:42:03.586565927  184302 grpclb.cc:864]              LB_POLICY: 0x619000002380   WEAK_UNREF 0x1 -> 0x0 [glb_rr_connectivity_cb]
D1116 19:42:03.586579057  184369 transport.cc:43]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80   REF 3->4 re-resolution
D1116 19:42:03.586605936  184369 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 4->3 resolver
D1116 19:42:03.586615146  184302 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 1->0 re-resolution
D1116 19:42:03.586623303  184369 subchannel_list.cc:44]      SUBCHANNEL: 0x616000006980 STRONG_UNREF 0x20001 -> 0x10002 [pf_shutdown]
D1116 19:42:03.586631791  184369 subchannel.cc:270]          SUBCHANNEL: 0x616000006980   WEAK_UNREF 0x10002 -> 0x10001 [strong-unref]
D1116 19:42:03.586640656  184369 transport.cc:58]            CLIENT_SUBCHANNEL 0x6120000082c0:0x6120000082c0 UNREF 2->1 pf_shutdown
D1116 19:42:03.586648973  184369 lb_policy.cc:72]            LB_POLICY: 0x60b000008bd0   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 19:42:03.586663066  184369 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 3->2 watch_lb_policy
D1116 19:42:03.586672248  184369 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 2->1 re-resolution
[       OK ] SingleBalancerTest.DropAll (1067 ms)
[----------] 1 test from SingleBalancerTest (1067 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1068 ms total)
[  PASSED  ] 1 test.
E1116 19:42:03.587815788  184302 backup_poller.cc:105]       run_poller: {"created":"@1510890123.587766925","description":"Shutting down timer system","file":"src/core/lib/iomgr/timer_generic.cc","file_line":635}
D1116 19:42:04.588478145  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:05.589200435  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:06.589989791  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:07.590814491  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:08.591620719  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:09.592498756  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:10.594325628  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:11.595336633  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:12.596161258  184302 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 19:42:13.596971495  184302 iomgr.cc:125]               Failed to free 1 iomgr objects before shutdown deadline: memory leaks are likely
D1116 19:42:13.596996987  184302 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:20872 fd=18 0x6100000065e0
D1116 19:42:13.597032253  184302 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 19:42:13.597047321  184302 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 19:42:13.597073105  184302 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 19:42:13.597088172  184302 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:42:13.597113973  184302 slice_intern.cc:325]        LEAKED: 67 72 70 63 2d 63 2b 2b 2f 31 2e 38 2e 30 2d 64 65 76 20 67 72 70 63 2d 63 2f 35 2e 30 2e 30 2d 64 65 76 20 28 6c 69 6e 75 78 3b 20 63 68 74 74 70 32 3b 20 67 65 6e 65 72 6f 75 73 29 'grpc-c++/1.8.0-dev grpc-c/5.0.0-dev (linux; chttp2; generous)'
D1116 19:42:13.597127768  184302 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:42:13.597141177  184302 slice_intern.cc:325]        LEAKED: 73 65 72 76 65 72 6e 61 6d 65 5f 6e 6f 74 5f 75 73 65 64 'servername_not_used'
D1116 19:42:13.597419789  184302 ev_posix.cc:135]            Using polling engine: poll
D1116 19:42:13.597468969  184302 dns_resolver.cc:307]        Using native dns resolver

=================================================================
^[[1m^[[31m==184302==ERROR: LeakSanitizer: detected memory leaks
^[[1m^[[0m
^[[1m^[[34mDirect leak of 48 byte(s) in 1 object(s) allocated from:
^[[1m^[[0m    #0 0x4c15ab in __interceptor_malloc /home/development/llvm/3.8.1/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x80f077 in gpr_malloc /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:56:7
    #2 0x91a407 in new_node(void*, void*, gpr_avl_node*, gpr_avl_node*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:81:39
    #3 0x91df56 in rebalance(gpr_avl_vtable const*, void*, void*, gpr_avl_node*, gpr_avl_node*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:196:32
    #4 0x91d1e7 in remove_key(gpr_avl_vtable const*, gpr_avl_node*, void*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:276:12
    #5 0x91cd20 in remove_key(gpr_avl_vtable const*, gpr_avl_node*, void*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:267:24
    #6 0x91c30f in gpr_avl_remove /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:285:14
    #7 0x8e9397 in grpc_subchannel_index_unregister /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel_index.cc:235:9
    #8 0x8dec49 in disconnect(grpc_exec_ctx*, grpc_subchannel*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel.cc:246:3
    #9 0x8dea93 in grpc_subchannel_unref /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/subchannel.cc:268:5
    #10 0xa3af11 in grpc_lb_subchannel_data_unref_subchannel /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc:44:5

For the following run, the refcounting shows that only the LB policy 0x60b000008910 is leaked.
D1116 17:57:50.547259312  103511 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 5->4 start_transport_op
[       OK ] UpdatesTest.UpdateBalancersDeadUpdate (129 ms)
D1116 17:57:50.547274004  103511 subchannel_list.cc:228]     LB_POLICY: 0x60b000008910   WEAK_UNREF 0x3 -> 0x2 [pf_selected_shutdown]
[----------] 1 test from UpdatesTest (129 ms total)

D1116 17:57:50.547297612  103511 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 4->3 external_connectivity_watcher
[----------] Global test environment tear-down
D1116 17:57:50.547320043  103511 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 3->2 resolver
[==========] 1 test from 1 test case ran. (129 ms total)
[  PASSED  ] 1 test.
D1116 17:57:50.547355875  103511 subchannel_list.cc:44]      SUBCHANNEL: 0x616000013b80 STRONG_UNREF 0x10001 -> 0x2 [pf_shutdown]
D1116 17:57:50.547437272  103511 subchannel_index.cc:111]    SUBCHANNEL: 0x616000006980     WEAK_REF 0x10001 -> 0x10002 [subchannel_index]
D1116 17:57:50.547473672  103511 subchannel_index.cc:107]    SUBCHANNEL: 0x616000006980   WEAK_UNREF 0x10002 -> 0x10001 [subchannel_index]
D1116 17:57:50.547494065  103511 subchannel_index.cc:107]    SUBCHANNEL: 0x616000013b80   WEAK_UNREF 0x2 -> 0x1 [subchannel_index]
D1116 17:57:50.547518441  103511 transport.cc:58]            CLIENT_SUBCHANNEL 0x612000022240:0x612000022240 UNREF 2->1 connection
D1116 17:57:50.547532889  103511 subchannel.cc:270]          SUBCHANNEL: 0x616000013b80   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 17:57:50.547546095  103511 transport.cc:58]            CLIENT_SUBCHANNEL 0x612000022240:0x612000022240 UNREF 1->0 pf_shutdown
D1116 17:57:50.547565837  103511 lb_policy.cc:72]            LB_POLICY: 0x60b000008910   WEAK_UNREF 0x2 -> 0x1 [strong-unref]
D1116 17:57:50.547604528  103511 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 2->1 re-resolution
D1116 17:57:50.547618401  103511 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 1->0 watch_lb_policy
D1116 17:57:50.547646543  103511 grpclb.cc:1871]             LB_POLICY: 0x619000007880   WEAK_UNREF 0x1 -> 0x0 [watch_lb_channel_connectivity_cb_shutdown]
E1116 17:57:50.548003973  103252 backup_poller.cc:105]       run_poller: {"created":"@1510883870.547956277","description":"Shutting down timer system","file":"src/core/lib/iomgr/timer_generic.cc","file_line":635}
D1116 17:57:51.548775962  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:52.549524901  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:53.550294663  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:54.551077505  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:55.551873345  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:56.552667510  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:57.553458596  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:58.554706839  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:57:59.555506177  103252 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1116 17:58:00.556255522  103252 iomgr.cc:125]               Failed to free 1 iomgr objects before shutdown deadline: memory leaks are likely
D1116 17:58:00.556277700  103252 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:32179 fd=26 0x60d00000be08
D1116 17:58:00.556324758  103252 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 17:58:00.556339672  103252 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 17:58:00.556354882  103252 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 17:58:00.556375676  103252 slice_intern.cc:325]        LEAKED: 67 72 70 63 2d 63 2b 2b 2f 31 2e 38 2e 30 2d 64 65 76 20 67 72 70 63 2d 63 2f 35 2e 30 2e 30 2d 64 65 76 20 28 6c 69 6e 75 78 3b 20 63 68 74 74 70 32 3b 20 67 65 6e 65 72 6f 75 73 29 'grpc-c++/1.8.0-dev grpc-c/5.0.0-dev (linux; chttp2; generous)'
D1116 17:58:00.556648749  103252 ev_posix.cc:135]            Using polling engine: epollsig
D1116 17:58:00.556703761  103252 dns_resolver.cc:307]        Using native dns resolver

=================================================================
^[[1m^[[31m==103252==ERROR: LeakSanitizer: detected memory leaks
^[[1m^[[0m
^[[1m^[[34mDirect leak of 48 byte(s) in 1 object(s) allocated from:
^[[1m^[[0m    #0 0x4c15ab in __interceptor_malloc /home/development/llvm/3.8.1/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x80f077 in gpr_malloc /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:56:7
    #2 0x91a407 in new_node(void*, void*, gpr_avl_node*, gpr_avl_node*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:81:39
    #3 0x91df56 in rebalance(gpr_avl_vtable const*, void*, void*, gpr_avl_node*, gpr_avl_node*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:196:32
    #4 0x91cfd1 in remove_key(gpr_avl_vtable const*, gpr_avl_node*, void*, void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/avl.cc:271:12

The refcounting shows that only the client channel 0x6160000078f8:0x616000007880 was leaked and all the three LB policies are not, but the stacktrace indicates that a glb_policy was leaked indirectly.
D1116 18:29:21.018547723  128536 client_channel.cc:673]      LB_POLICY: 0x61900000be80 STRONG_UNREF 0x10002 -> 0x3 [channel]
D1116 18:29:21.018562058  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000078f8:0x616000007880 UNREF 4->3 start_transport_op
D1116 18:29:21.018575981  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000078f8:0x616000007880 UNREF 3->2 watch_lb_policy
D1116 18:29:21.018850119  128536 transport.cc:43]            CLIENT_CHANNEL 0x6160000078f8:0x616000007880   REF 2->3 re-resolution
D1116 18:29:21.018884473  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000078f8:0x616000007880 UNREF 3->2 resolver
D1116 18:29:21.018904207  128536 grpclb.cc:1027]             LB_POLICY: 0x60b0000068c0 STRONG_UNREF 0x10000 -> 0x1 [glb_shutdown]
D1116 18:29:21.018918577  128536 transport.cc:43]            CLIENT_CHANNEL 0x6160000075f8:0x616000007580   REF 4->5 start_transport_op
D1116 18:29:21.018927031  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000075f8:0x616000007580 UNREF 5->4 channel
D1116 18:29:21.018935555  128536 lb_policy.cc:72]            LB_POLICY: 0x61900000be80   WEAK_UNREF 0x3 -> 0x2 [strong-unref]
D1116 18:29:21.018948797  128536 grpclb.cc:1286]             LB_POLICY: 0x61900000be80   WEAK_UNREF 0x2 -> 0x1 [grpclb_retry_timer]
D1116 18:29:21.018968318  128536 lb_policy.cc:72]            LB_POLICY: 0x60b0000068c0   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 18:29:21.018980904  128536 grpclb.cc:864]              LB_POLICY: 0x61900000be80   WEAK_UNREF 0x1 -> 0x0 [glb_rr_connectivity_cb]
D1116 18:29:21.019002458  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000078f8:0x616000007880 UNREF 2->1 re-resolution
D1116 18:29:21.019085636  128536 subchannel_list.cc:228]     LB_POLICY: 0x60b000008de0   WEAK_UNREF 0x10001 -> 0x10000 [pf_selected_shutdown]
D1116 18:29:21.019116947  128536 client_channel.cc:673]      LB_POLICY: 0x60b000008de0 STRONG_UNREF 0x10000 -> 0x1 [channel]
D1116 18:29:21.019128607  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000075f8:0x616000007580 UNREF 4->3 start_transport_op
D1116 18:29:21.019140984  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000075f8:0x616000007580 UNREF 3->2 re-resolution
D1116 18:29:21.019155625  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000075f8:0x616000007580 UNREF 2->1 watch_lb_policy
D1116 18:29:21.019168189  128536 transport.cc:58]            CLIENT_CHANNEL 0x6160000075f8:0x616000007580 UNREF 1->0 resolver
D1116 18:29:21.019189555  128536 subchannel_list.cc:44]      SUBCHANNEL: 0x616000007280 STRONG_UNREF 0x10001 -> 0x2 [pf_shutdown]
D1116 18:29:21.019211969  128536 subchannel_index.cc:107]    SUBCHANNEL: 0x616000007280   WEAK_UNREF 0x2 -> 0x1 [subchannel_index]
D1116 18:29:21.019225007  128536 transport.cc:58]            CLIENT_SUBCHANNEL 0x612000009dc0:0x612000009dc0 UNREF 2->1 connection
D1116 18:29:21.019234497  128536 subchannel.cc:270]          SUBCHANNEL: 0x616000007280   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 18:29:21.019243332  128536 transport.cc:58]            CLIENT_SUBCHANNEL 0x612000009dc0:0x612000009dc0 UNREF 1->0 pf_shutdown
D1116 18:29:21.019252062  128536 lb_policy.cc:72]            LB_POLICY: 0x60b000008de0   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
E1116 18:29:21.019511276  128534 backup_poller.cc:105]       run_poller: {"created":"@1510885761.019482774","description":"Timer list shutdown","file":"src/core/lib/iomgr/timer_generic.cc","file_line":279}
D1116 18:29:21.019540290  128534 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 18:29:21.019551053  128534 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 18:29:21.019556715  128534 metadata.cc:133]            WARNING: 2 metadata elements were leaked
D1116 18:29:21.019563678  128534 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 18:29:21.019570737  128534 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 18:29:21.019578275  128534 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 18:29:21.019591083  128534 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 45 63 68 6f '/grpc.testing.EchoTestService/Echo'
D1116 18:29:21.019597999  128534 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 18:29:21.019609510  128534 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 55 6e 69 6d 70 6c 65 6d 65 6e 74 65 64 '/grpc.testing.EchoTestService/Unimplemented'
D1116 18:29:21.019614768  128534 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 18:29:21.019624016  128534 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 52 65 73 70 6f 6e 73 65 53 74 72 65 61 6d '/grpc.testing.EchoTestService/ResponseStream'
D1116 18:29:21.019629993  128534 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 18:29:21.019636807  128534 slice_intern.cc:325]        LEAKED: 73 65 72 76 65 72 6e 61 6d 65 5f 6e 6f 74 5f 75 73 65 64 'servername_not_used'
D1116 18:29:21.019646863  128534 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 18:29:21.019659377  128534 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 42 69 64 69 53 74 72 65 61 6d '/grpc.testing.EchoTestService/BidiStream'
D1116 18:29:21.019669047  128534 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 18:29:21.019687683  128534 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 52 65 71 75 65 73 74 53 74 72 65 61 6d '/grpc.testing.EchoTestService/RequestStream'
D1116 18:29:21.019884537  128534 ev_posix.cc:135]            Using polling engine: epollsig
D1116 18:29:21.019921446  128534 dns_resolver.cc:307]        Using native dns resolver

=================================================================
^[[1m^[[31m==128534==ERROR: LeakSanitizer: detected memory leaks
^[[1m^[[0m
^[[1m^[[34mIndirect leak of 1024 byte(s) in 1 object(s) allocated from:
^[[1m^[[0m    #0 0x4c1734 in calloc /home/development/llvm/3.8.1/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:66:3
    #1 0x80f7d6 in zalloc_with_calloc(unsigned long) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:27:53
    #2 0x80f227 in gpr_zalloc /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:68:7
    #3 0xa16232 in glb_create(grpc_exec_ctx*, grpc_lb_policy_factory*, grpc_lb_policy_args*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc:1920:47
    #4 0x718d42 in grpc_lb_policy_factory_create_lb_policy /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy_factory.cc:168:10
    #5 0x8db54a in grpc_lb_policy_create /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy_registry.cc:68:7
    #6 0x8d341f in on_resolver_result_changed_locked(grpc_exec_ctx*, void*, grpc_error*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/client_channel.cc:465:11
    #7 0x6b49f3 in grpc_combiner_continue_exec_ctx /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/combiner.cc:261:5
    #8 0x6c5be0 in grpc_exec_ctx_flush /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/exec_ctx.cc:96:17
    #9 0x755752 in pollset_work(grpc_exec_ctx*, grpc_pollset*, grpc_pollset_worker**, long) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_epollsig_linux.cc:1367:5
    #10 0x6c464c in grpc_pollset_work /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_posix.cc:238:10
    #11 0xa0ba81 in run_poller(grpc_exec_ctx*, void*, grpc_error*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/backup_poller.cc:116:21
    #12 0x6c61fb in exec_ctx_run(grpc_exec_ctx*, grpc_closure*, grpc_error*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/exec_ctx.cc:73:3
    #13 0x6c5b7a in grpc_exec_ctx_flush /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/exec_ctx.cc:93:9
    #14 0x6fc4a1 in run_some_timers(grpc_exec_ctx*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/timer_manager.cc:129:3
    #15 0x6fbca1 in timer_main_loop(grpc_exec_ctx*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/timer_manager.cc:236:9
    #16 0x6fba06 in timer_thread(void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/timer_manager.cc:284:3
    #17 0x81aa24 in thread_body(void*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/thd_posix.cc:42:3
    #18 0x7fbd9daf9183 in start_thread /build/eglibc-SvCtMH/eglibc-2.19/nptl/pthread_create.c:312

Like the previous run, the refcounting shows that only client channel 0x616000006ff8:0x616000006f80 was leaked, but the stacktrace shows a glb_policy was leaked. Also,  LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>: shows that some balancer servers are leaked.
I1116 19:29:56.655896845  172139 grpclb_end2end_test.cc:527] starting backend server on port 29243
I1116 19:29:56.656244884  172194 server_builder.cc:269]      Synchronous server. Num CQs: 1, Min pollers: 1, Max Pollers: 2, CQ timeout (msec): 10000
I1116 19:29:56.657977603  172139 grpclb_end2end_test.cc:532] backend server startup complete
I1116 19:29:56.659775665  172139 grpclb_end2end_test.cc:527] starting backend server on port 20440
I1116 19:29:56.660022507  172212 server_builder.cc:269]      Synchronous server. Num CQs: 1, Min pollers: 1, Max Pollers: 2, CQ timeout (msec): 10000
I1116 19:29:56.661377142  172139 grpclb_end2end_test.cc:532] backend server startup complete
I1116 19:29:56.663773806  172139 grpclb_end2end_test.cc:527] starting backend server on port 30265
I1116 19:29:56.664020926  172227 server_builder.cc:269]      Synchronous server. Num CQs: 1, Min pollers: 1, Max Pollers: 2, CQ timeout (msec): 10000
I1116 19:29:56.664601840  172139 grpclb_end2end_test.cc:532] backend server startup complete
I1116 19:29:56.665884390  172139 grpclb_end2end_test.cc:527] starting backend server on port 5050
I1116 19:29:56.666137470  172231 server_builder.cc:269]      Synchronous server. Num CQs: 1, Min pollers: 1, Max Pollers: 2, CQ timeout (msec): 10000
I1116 19:29:56.666753417  172139 grpclb_end2end_test.cc:532] backend server startup complete
I1116 19:29:56.668967231  172139 grpclb_end2end_test.cc:527] starting balancer server on port 9259
I1116 19:29:56.669294280  172244 server_builder.cc:269]      Synchronous server. Num CQs: 1, Min pollers: 1, Max Pollers: 2, CQ timeout (msec): 10000
I1116 19:29:56.669798972  172139 grpclb_end2end_test.cc:532] balancer server startup complete
.
.
.
.
D1116 19:29:58.686593693  172252 lb_policy.cc:72]            LB_POLICY: 0x61900000f580   WEAK_UNREF 0x3 -> 0x2 [strong-unref]
D1116 19:29:58.686600646  172252 grpclb.cc:1286]             LB_POLICY: 0x61900000f580   WEAK_UNREF 0x2 -> 0x1 [grpclb_retry_timer]
D1116 19:29:58.686611179  172252 lb_policy.cc:72]            LB_POLICY: 0x60b000006600   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 19:29:58.686620458  172252 grpclb.cc:864]              LB_POLICY: 0x61900000f580   WEAK_UNREF 0x1 -> 0x0 [glb_rr_connectivity_cb]
D1116 19:29:58.686642148  172252 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 2->1 re-resolution
D1116 19:29:58.686662101  172252 subchannel_list.cc:228]     LB_POLICY: 0x60b000008d30   WEAK_UNREF 0x10001 -> 0x10000 [pf_selected_shutdown]
D1116 19:29:58.686681351  172252 client_channel.cc:673]      LB_POLICY: 0x60b000008d30 STRONG_UNREF 0x10000 -> 0x1 [channel]
D1116 19:29:58.686689055  172252 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 4->3 start_transport_op
D1116 19:29:58.686695831  172252 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 3->2 re-resolution
D1116 19:29:58.686704771  172252 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 2->1 resolver
D1116 19:29:58.686716467  172252 subchannel_list.cc:44]      SUBCHANNEL: 0x616000006980 STRONG_UNREF 0x10001 -> 0x2 [pf_shutdown]
D1116 19:29:58.686732393  172252 subchannel_index.cc:107]    SUBCHANNEL: 0x616000006980   WEAK_UNREF 0x2 -> 0x1 [subchannel_index]
D1116 19:29:58.686744562  172252 transport.cc:58]            CLIENT_SUBCHANNEL 0x6120000082c0:0x6120000082c0 UNREF 2->1 connection
D1116 19:29:58.686751432  172252 subchannel.cc:270]          SUBCHANNEL: 0x616000006980   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 19:29:58.686758687  172252 transport.cc:58]            CLIENT_SUBCHANNEL 0x6120000082c0:0x6120000082c0 UNREF 1->0 pf_shutdown
D1116 19:29:58.686765974  172252 lb_policy.cc:72]            LB_POLICY: 0x60b000008d30   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1116 19:29:58.686791789  172252 transport.cc:58]            CLIENT_CHANNEL 0x616000006cf8:0x616000006c80 UNREF 1->0 watch_lb_policy
E1116 19:29:58.686997680  172139 backup_poller.cc:105]       run_poller: {"created":"@1510889398.686966486","description":"Shutting down timer system","file":"src/core/lib/iomgr/timer_generic.cc","file_line":635}
D1116 19:29:59.687721493  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:00.689008553  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:01.690005318  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:02.690795882  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:03.691557079  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:04.692651566  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:05.693428849  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:06.694239396  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:07.695862581  172139 iomgr.cc:95]                Waiting for 5 iomgr objects to be destroyed
D1116 19:30:08.696631668  172139 iomgr.cc:125]               Failed to free 5 iomgr objects before shutdown deadline: memory leaks are likely
D1116 19:30:08.696648575  172139 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:9259 fd=18 0x6100000065e0
D1116 19:30:08.696656129  172139 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:29243 fd=22 0x6100000064e0
D1116 19:30:08.696661693  172139 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:20440 fd=24 0x6100000063e0
D1116 19:30:08.696666329  172139 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:30265 fd=25 0x6100000062e0
D1116 19:30:08.696671953  172139 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:5050 fd=26 0x6100000061e0
D1116 19:30:08.696704364  172139 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 19:30:08.696714966  172139 metadata.cc:133]            WARNING: 2 metadata elements were leaked
D1116 19:30:08.696721526  172139 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 19:30:08.696730666  172139 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 19:30:08.696736515  172139 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1116 19:30:08.696746067  172139 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:30:08.696768516  172139 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 52 65 71 75 65 73 74 53 74 72 65 61 6d '/grpc.testing.EchoTestService/RequestStream'
D1116 19:30:08.696777247  172139 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:30:08.696787314  172139 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 55 6e 69 6d 70 6c 65 6d 65 6e 74 65 64 '/grpc.testing.EchoTestService/Unimplemented'
D1116 19:30:08.696793226  172139 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:30:08.696804619  172139 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 42 69 64 69 53 74 72 65 61 6d '/grpc.testing.EchoTestService/BidiStream'
D1116 19:30:08.696810835  172139 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:30:08.696819031  172139 slice_intern.cc:325]        LEAKED: 73 65 72 76 65 72 6e 61 6d 65 5f 6e 6f 74 5f 75 73 65 64 'servername_not_used'
D1116 19:30:08.696827462  172139 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:30:08.696845812  172139 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 45 63 68 6f '/grpc.testing.EchoTestService/Echo'
D1116 19:30:08.696863609  172139 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1116 19:30:08.696877117  172139 slice_intern.cc:325]        LEAKED: 2f 67 72 70 63 2e 74 65 73 74 69 6e 67 2e 45 63 68 6f 54 65 73 74 53 65 72 76 69 63 65 2f 52 65 73 70 6f 6e 73 65 53 74 72 65 61 6d '/grpc.testing.EchoTestService/ResponseStream'
D1116 19:30:08.697135297  172139 ev_posix.cc:135]            Using polling engine: poll
D1116 19:30:08.697176837  172139 dns_resolver.cc:307]        Using native dns resolver

=================================================================
^[[1m^[[31m==172139==ERROR: LeakSanitizer: detected memory leaks
^[[1m^[[0m
^[[1m^[[34mIndirect leak of 1024 byte(s) in 1 object(s) allocated from:
^[[1m^[[0m    #0 0x4c1734 in calloc /home/development/llvm/3.8.1/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:66:3
    #1 0x80f7d6 in zalloc_with_calloc(unsigned long) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:27:53
    #2 0x80f227 in gpr_zalloc /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/support/alloc.cc:68:7
    #3 0xa16232 in glb_create(grpc_exec_ctx*, grpc_lb_policy_factory*, grpc_lb_policy_args*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc:1920:47
    #4 0x718d42 in grpc_lb_policy_factory_create_lb_policy /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy_factory.cc:168:10
    #5 0x8db54a in grpc_lb_policy_create /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/lb_policy_registry.cc:68:7
    #6 0x8d341f in on_resolver_result_changed_locked(grpc_exec_ctx*, void*, grpc_error*) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/ext/filters/client_channel/client_channel.cc:465:11
    #7 0x6b49f3 in grpc_combiner_continue_exec_ctx /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/combiner.cc:261:5
    #8 0x6c5be0 in grpc_exec_ctx_flush /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/exec_ctx.cc:96:17
    #9 0x7670b1 in pollset_work(grpc_exec_ctx*, grpc_pollset*, grpc_pollset_worker**, long) /usr/local/google/home/juanlishen/wip/reresolution/grpc/src/core/lib/iomgr/ev_poll_posix.cc:1057:22

After canceling the closure whenever we notice we are shutting down, the client channel is not leaked. But the LB policy leak remains.
 The following log shows how the re-resolution closure of LB 0x60b000000e00 leads to such a leak.
D1121 15:48:37.832284764   43286 grpclb.cc:864]              LB_POLICY: 0x61900001ef80   WEAK_UNREF 0x2 -> 0x1 [glb_rr_connectivity_cb]
D1121 15:48:37.832294149   43286 transport.cc:58]            CLIENT_CHANNEL 0x6160000072f8:0x616000007280 UNREF 1->0 re-resolution
D1121 15:48:37.832301320   43286 client_channel.cc:383]      chand=0x616000007388, LB=0x61900001ef80: re-resolution unref
D1121 15:48:37.832301382   43299 subchannel_list.cc:221]     LB_POLICY: 0x60b000008230     WEAK_REF 0x10000 -> 0x10001 [connectivity_watch+replace_selected]
D1121 15:48:37.832308355   43286 client_channel.cc:812]      chand=0x616000007388, cc_destroy_channel_elem() started
D1121 15:48:37.832321281   43299 subchannel.cc:514]          SUBCHANNEL: 0x616000006c80     WEAK_REF 0x20001 -> 0x20002 [external_state_watcher]
D1121 15:48:37.832334440   43299 transport.cc:43]            CLIENT_SUBCHANNEL 0x612000009040:0x612000009040   REF 2->3 pf_update_includes_selected
D1121 15:48:37.832342979   43299 subchannel_list.cc:44]      SUBCHANNEL: 0x616000006c80 STRONG_UNREF 0x20002 -> 0x10003 [pf_update_includes_selected]
D1121 15:48:37.832349431   43299 subchannel.cc:270]          SUBCHANNEL: 0x616000006c80   WEAK_UNREF 0x10003 -> 0x10002 [strong-unref]
[       OK ] SingleBalancerWithClientLoadReportingTest.Vanilla (2026 ms)
D1121 15:48:37.832355612   43299 transport.cc:58]            CLIENT_SUBCHANNEL 0x612000009040:0x612000009040 UNREF 3->2 pf_update_includes_selected
D1121 15:48:37.832367291   43299 client_channel.cc:524]      chand=0x616000007088: resolver result: lb_policy_name="pick_first", service_config="(null)"
[----------] 1 test from SingleBalancerWithClientLoadReportingTest (2026 ms total)

[----------] Global test environment tear-down
D1121 15:48:37.832392395   43299 subchannel.cc:415]          SUBCHANNEL: 0x616000006c80   WEAK_UNREF 0x10002 -> 0x10001 [external_state_watcher]
D1121 15:48:37.832405296   43299 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 5->4 external_connectivity_watcher
[==========] 1 test from 1 test case ran. (2026 ms total)
[  PASSED  ] 1 test.
D1121 15:48:37.832413937   43299 client_channel.cc:260]      chand=0x616000007088: setting connectivity state to SHUTDOWN
D1121 15:48:37.832441303   43299 client_channel.cc:682]      LB_POLICY: 0x60b000008230 STRONG_UNREF 0x10001 -> 0x2 [channel]
D1121 15:48:37.832453720   43299 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 4->3 start_transport_op
D1121 15:48:37.832485583   43299 client_channel.cc:401]      chand=0x616000007088: got resolver result: error="No Error"
D1121 15:48:37.832542804   43299 transport.cc:43]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80   REF 3->4 re-resolution
D1121 15:48:37.832550499   43299 client_channel.cc:480]      chand=0x616000007088, LB=0x60b000000e00: re-resolution ref
************************************** A new LB policy 0x60b000000e00 is created, ref the client channel 0x616000006ff8:0x616000006f80.

D1121 15:48:37.832566977   43299 client_channel.cc:524]      chand=0x616000007088: resolver result: lb_policy_name="pick_first", service_config="(null)"
D1121 15:48:37.832573030   43299 client_channel.cc:575]      chand=0x616000007088, LB=0x60b000000e00: swapped in
************************************** The new LB policy is swapped in.

D1121 15:48:37.832578553   43299 client_channel.cc:582]      chand=0x616000007088: shutting down
D1121 15:48:37.832586819   43299 client_channel.cc:260]      chand=0x616000007088: setting connectivity state to SHUTDOWN
************************************** The client channel is shutting down.

D1121 15:48:37.832595625   43299 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 4->3 resolver
D1121 15:48:37.832608889   43299 subchannel_list.cc:228]     LB_POLICY: 0x60b000008230   WEAK_UNREF 0x2 -> 0x1 [pf_selected_shutdown]
D1121 15:48:37.832619749   43299 subchannel_list.cc:44]      SUBCHANNEL: 0x616000006c80 STRONG_UNREF 0x20001 -> 0x10002 [pf_shutdown]
D1121 15:48:37.832625891   43299 subchannel.cc:270]          SUBCHANNEL: 0x616000006c80   WEAK_UNREF 0x10002 -> 0x10001 [strong-unref]
D1121 15:48:37.832633994   43299 transport.cc:58]            CLIENT_SUBCHANNEL 0x612000009040:0x612000009040 UNREF 2->1 pf_shutdown
D1121 15:48:37.832641991   43299 lb_policy.cc:72]            LB_POLICY: 0x60b000008230   WEAK_UNREF 0x1 -> 0x0 [strong-unref]
D1121 15:48:37.832652127   43299 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 3->2 re-resolution
D1121 15:48:37.832657501   43299 client_channel.cc:383]      chand=0x616000007088, LB=0x60b000008230: re-resolution unref
D1121 15:48:37.832663570   43299 transport.cc:58]            CLIENT_CHANNEL 0x616000006ff8:0x616000006f80 UNREF 2->1 watch_lb_policy
************************************** The ref count of client channel 0x616000006ff8:0x616000006f80 remains 1, so the client channel is leaked.

************************************** Although we can't see any refcounting for LB policy 0x60b000000e00, it's also leaked because we don't ever unref it. (When the LB policy is created, its ref count is 1, but we don't have a log for that. I think that's also why I can't understand some previous logs.)

************************************** Also, there is a log saying "chand=0x616000007388, cc_destroy_channel_elem() started", but there isn't such one for chand=0x616000007088, so cc_destroy_channel_elem() is not invoked for client channel 0x616000006ff8:0x616000006f80.

D1121 15:48:37.832673978   43299 grpclb.cc:1871]             LB_POLICY: 0x61900001ef80   WEAK_UNREF 0x1 -> 0x0 [watch_lb_channel_connectivity_cb_shutdown]
E1121 15:48:37.832851549   43286 backup_poller.cc:105]       run_poller: {"created":"@1511308117.832823862","description":"Shutting down timer system","file":"src/core/lib/iomgr/timer_generic.cc","file_line":635}
D1121 15:48:38.833613326   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:39.834368037   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:40.835164265   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:41.835924745   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:42.836695087   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:43.837454110   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:44.838164395   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:45.839101176   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:46.839914585   43286 iomgr.cc:95]                Waiting for 1 iomgr objects to be destroyed
D1121 15:48:47.840778503   43286 iomgr.cc:125]               Failed to free 1 iomgr objects before shutdown deadline: memory leaks are likely
D1121 15:48:47.840800887   43286 iomgr.cc:75]                LEAKED OBJECT: tcp-client:ipv4:<IP_ADDRESS>:9361 fd=20 0x60d00000c078
D1121 15:48:47.840849222   43286 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1121 15:48:47.840873204   43286 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1121 15:48:47.840885512   43286 metadata.cc:133]            WARNING: 1 metadata elements were leaked
D1121 15:48:47.840901761   43286 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1121 15:48:47.840930906   43286 slice_intern.cc:325]        LEAKED: 67 72 70 63 2d 63 2b 2b 2f 31 2e 38 2e 30 2d 64 65 76 20 67 72 70 63 2d 63 2f 35 2e 30 2e 30 2d 64 65 76 20 28 6c 69 6e 75 78 3b 20 63 68 74 74 70 32 3b 20 67 65 6e 65 72 6f 75 73 29 'grpc-c++/1.8.0-dev grpc-c/5.0.0-dev (linux; chttp2; generous)'
D1121 15:48:47.840947494   43286 slice_intern.cc:318]        WARNING: 1 metadata strings were leaked
D1121 15:48:47.840964475   43286 slice_intern.cc:325]        LEAKED: 73 65 72 76 65 72 6e 61 6d 65 5f 6e 6f 74 5f 75 73 65 64 'servername_not_used'
D1121 15:48:47.841238763   43286 ev_posix.cc:135]            Using polling engine: epollsig
D1121 15:48:47.841283161   43286 dns_resolver.cc:307]        Using native dns resolver

=================================================================
^[[1m^[[31m==43286==ERROR: LeakSanitizer: detected memory leaks

So this is a circular dependency, and we can avoid this by making sure cc_destroy_channel_elem() doesn't have any chand->lb_policy to unref, or at least the chand->lb_policy doesn't have a pending re-resolution closure to cancel.
The last commit here is to avoid creating the re-resolution closure and giving it to the new LB policy when the client channel is shutting down. #13472 is to unref the whole LB policy when the client channel is shutting down. Both solutions can achieve the goal.
I couldn't understand the log when I only saw the client channel leak. After all, there's a circular dependency. But now I think the LB policy was also leaked. I didn't see it because the refcounting for the leaked LB policy never appears.
After either fix is in, I still see a leak of LB policy. I think this leak is different from the circular dependency one. From the refcounting, I don't see a matching unref for lb_on_server_status_received_locked. Still looking at this one.

Below is the full list of the refcounting for the leaked LB policy after the circular reference is fixed.
We ref for lb_on_server_status_received_locked at the beginning but never unref for it.

Now the leak I see is about pick first policy. Looking.

Thanks for diving into this in such detail -- the trace you showed is a good analysis of the problem!

The last commit here is to avoid creating the re-resolution closure and giving it to the new LB policy when the client channel is shutting down. #13472 is to unref the whole LB policy when the client channel is shutting down. Both solutions can achieve the goal.

I think the fix I suggested in #13472 is the right one: we should not create the new LB policy in the first place if we are shutting down.  Once that change is made, I suggest reverting the last commit on this PR; if we do create a new LB policy, I think it makes sense to set the reresolution closure as soon as possible.

Reviewed 4 of 4 files at r19, 1 of 1 files at r21.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from Reviewable

Regarding to the leak about pick first, I think the bug is that we don't unref for connectivity watch when shutting down the subchannel list from inside of pf_connectivity_changed_locked().
 13:24:35.778117514  117940 subchannel_list.cc:147]     [pick_first 0x60b000008f40] subchannel list 0x608000029a20 index 1: Created subchannel 0x61600004f580 for address uri ipv4:<IP_ADDRESS>:4938
D1127 13:24:35.778133224  117938 grpclb.cc:691]              LB_POLICY: 0x60b000006ad0 STRONG_UNREF 0x20001 -> 0x10002 [glb_pick_sync]
D1127 13:24:35.778207502  117940 subchannel_list.cc:147]     [pick_first 0x60b000008f40] subchannel list 0x608000029a20 index 2: Created subchannel 0x61600004f280 for address uri ipv4:<IP_ADDRESS>:16775
I1127 13:24:35.778216926  117940 pick_first.cc:299]          Pick First 0x60b000008f40 found already selected subchannel 0x616000006980 at update index 0 of 3; update done
D1127 13:24:35.778223146  117940 subchannel_list.cc:255]     [pick_first 0x60b000008f40] Shutting down subchannel_list 0x608000029ea0 (pf_update_includes_selected)
D1127 13:24:35.778228979  117940 subchannel_list.cc:37]      [pick_first 0x60b000008f40] subchannel list 0x608000029ea0 index 0 of 3 (subchannel 0x616000006980): unreffing subchannel
D1127 13:24:35.778234977  117940 subchannel_list.cc:211]     [pick_first 0x60b000008f40] subchannel_list 0x608000029ea0 UNREF 1->0 (pf_update_includes_selected)
D1127 13:24:35.778240005  117940 subchannel_list.cc:180]     [pick_first 0x60b000008f40] Destroying subchannel_list 0x608000029ea0
D1127 13:24:35.778245980  117940 subchannel_list.cc:37]      [pick_first 0x60b000008f40] subchannel list 0x608000029a20 index 1 of 3 (subchannel 0x61600004f580): unreffing subchannel
D1127 13:24:35.778273319  117940 subchannel_list.cc:37]      [pick_first 0x60b000008f40] subchannel list 0x608000029a20 index 2 of 3 (subchannel 0x61600004f280): unreffing subchannel
D1127 13:24:35.778299775  117940 subchannel_list.cc:223]     LB_POLICY: 0x60b000008f40     WEAK_REF 0x10000 -> 0x10001 [connectivity_watch+replace_selected]
D1127 13:24:35.778309010  117940 subchannel_list.cc:198]     [pick_first 0x60b000008f40] subchannel_list 0x608000029a20 REF 1->2 (connectivity_watch+replace_selected)
D1127 13:24:35.778315635  117940 subchannel_list.cc:63]      [pick_first 0x60b000008f40] subchannel list 0x608000029a20 index 0 of 3 (subchannel 0x616000006980, closure 0x614000019060): requesting connectivity change notification
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx Subchannel 0x616000006980 has a pending connecttivity notification,

I1127 13:24:35.778321671  117940 subchannel.cc:525]          ABOUT TO PRINT THE WATCHER LIST
I1127 13:24:35.778328726  117940 subchannel.cc:529]          SUBCHANNEL 0x616000006980, WATCHER 0x60c000027f40, NOTIFY 0x614000019060
D1127 13:24:35.778334821  117940 connectivity_state.cc:118]  CONWATCH: 0x616000006aa0 subchannel: from IDLE [cur=SHUTDOWN] notify=0x60c000027f58
D1127 13:24:35.778374087  117940 connectivity_state.cc:118]  CONWATCH: 0x616000006e28 client_channel: from TRANSIENT_FAILURE [cur=TRANSIENT_FAILURE] notify=0x60d00000b400
I1127 13:24:35.778387724  117940 pick_first.cc:262]          Pick First 0x60b000008f40 received update with 1 addresses
D1127 13:24:35.778394297  117940 subchannel_list.cc:101]     [pick_first 0x60b000008f40] Creating subchannel list 0x608000029720 for 1 subchannels
D1127 13:24:35.778478167  117940 subchannel_list.cc:147]     [pick_first 0x60b000008f40] subchannel list 0x608000029720 index 0: Created subchannel 0x61600004ef80 for address uri ipv4:<IP_ADDRESS>:4938
D1127 13:24:35.778494559  117940 pick_first.cc:349]          Pick First 0x60b000008f40 updated latest_pending_subchannel_list to 0x608000029720, waiting for it to be ready
D1127 13:24:35.778508391  117940 subchannel_list.cc:223]     LB_POLICY: 0x60b000008f40     WEAK_REF 0x10001 -> 0x10002 [connectivity_watch+update]
D1127 13:24:35.778523840  117940 subchannel_list.cc:198]     [pick_first 0x60b000008f40] subchannel_list 0x608000029720 REF 1->2 (connectivity_watch+update)
D1127 13:24:35.778536270  117940 subchannel_list.cc:63]      [pick_first 0x60b000008f40] subchannel list 0x608000029720 index 0 of 1 (subchannel 0x61600004ef80, closure 0x60d0000173b0): requesting connectivity change notification
I1127 13:24:35.778548254  117940 subchannel.cc:525]          ABOUT TO PRINT THE WATCHER LIST
I1127 13:24:35.778558973  117940 subchannel.cc:529]          SUBCHANNEL 0x61600004ef80, WATCHER 0x60c000027e80, NOTIFY 0x60d0000173b0
D1127 13:24:35.778573211  117940 connectivity_state.cc:118]  CONWATCH: 0x61600004f0a0 subchannel: from IDLE [cur=IDLE] notify=0x60c000027e98
D1127 13:24:35.778588236  117940 connectivity_state.cc:169]  SET: 0x61600004f0a0 subchannel: IDLE --> CONNECTING [state_change] error=(nil) "No Error"
D1127 13:24:35.778151906  117938 grpclb.cc:691]              LB_POLICY: 0x60b000006ad0   WEAK_UNREF 0x10002 -> 0x10001 [strong-unref]
D1127 13:24:35.778599707  117940 connectivity_state.cc:195]  NOTIFY: 0x61600004f0a0 subchannel: 0x60c000027e98
D1127 13:24:35.778622650  117938 client_channel.cc:1184]     LB_POLICY: 0x619000007880 STRONG_UNREF 0x20003 -> 0x10004 [pick_subchannel]
D1127 13:24:35.778636647  117938 client_channel.cc:1184]     LB_POLICY: 0x619000007880   WEAK_UNREF 0x10004 -> 0x10003 [strong-unref]
D1127 13:24:35.779326286  117938 client_channel.cc:1171]     LB_POLICY: 0x619000007880   STRONG_REF 0x10003 -> 0x20003 [pick_subchannel]
D1127 13:24:35.779328384  117983 connectivity_state.cc:118]  CONWATCH: 0x6290000cd538 server_transport: from IDLE [cur=READY] notify=0x61700003fce8
D1127 13:24:35.779344638  117938 connectivity_state.cc:93]   CONWATCH: 0x60b000006b10 round_robin: get READY
D1127 13:24:35.779357028  117938 grpclb.cc:1209]             LB_POLICY: 0x60b000006ad0   STRONG_REF 0x10001 -> 0x20001 [glb_pick]
D1127 13:24:35.779364120  117983 connectivity_state.cc:118]  CONWATCH: 0x6290000cd538 server_transport: from READY [cur=READY] notify=0x61700003fce8
D1127 13:24:35.779370077  117938 grpclb.cc:691]              LB_POLICY: 0x60b000006ad0 STRONG_UNREF 0x20001 -> 0x10002 [glb_pick_sync]
D1127 13:24:35.779378807  117938 grpclb.cc:691]              LB_POLICY: 0x60b000006ad0   WEAK_UNREF 0x10002 -> 0x10001 [strong-unref]
D1127 13:24:35.779387000  117938 client_channel.cc:1184]     LB_POLICY: 0x619000007880 STRONG_UNREF 0x20003 -> 0x10004 [pick_subchannel]
D1127 13:24:35.779392630  117938 client_channel.cc:1184]     LB_POLICY: 0x619000007880   WEAK_UNREF 0x10004 -> 0x10003 [strong-unref]
D1127 13:24:35.779430233  117940 connectivity_state.cc:169]  SET: 0x61600004f0a0 subchannel: CONNECTING --> READY [connected] error=(nil) "No Error"
D1127 13:24:35.779461578  117940 pick_first.cc:369]          Pick First 0x60b000008f40 connectivity changed for subchannel 0x616000006980 (0 of 3), subchannel_list 0x608000029a20: state=SHUTDOWN p->shutdown=0 sd->subchannel_list->shutting_down=0 error={"created":"@1511817875.760229509","description":"GOAWAY received","file":"src/core/ext/transport/chttp2/transport/chttp2_transport.cc","file_line":1132,"http2_error":0,"raw_bytes":"Server shutdown"}
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx The connectivity of subchannel 0x616000006980 is changed to SHUTDOWN.

D1127 13:24:35.779479745  117940 subchannel_list.cc:255]     [pick_first 0x60b000008f40] Shutting down subchannel_list 0x608000029a20 (selected_not_ready+switch_to_update)
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx Because the selected subchannel is shut down, we switch to the update subchannel list and shut down the old subchannel list 0x608000029a20.

D1127 13:24:35.779500522  117940 subchannel_list.cc:237]     [pick_first 0x60b000008f40] subchannel list 0x608000029a20 index 0 of 3 (subchannel 0x616000006980): canceling connectivity watch (selected_not_ready+switch_to_update)
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx During shuting down, sd->connectivity_notification_pending is true for subchannel 0x616000006980, but we are still in the call stack of pf_connectivity_changed_locked() so the subchannel actually doesn't have any pending notification.

I1127 13:24:35.779518414  117940 subchannel_list.cc:245]     XXX ABOUT TO CANCEL CONNECTIVITY WATCH FOR SUBCHANNEL 0x616000006980. notify=0x614000019060
I1127 13:24:35.779531627  117940 subchannel.cc:495]          XXX YYY ABOUT TO CANCEL CONNECTIVITY WATCH FOR SUBCHANNEL 0x616000006980, STATE TRACKER 0x616000006aa0. INSIDE STATE == NULLPTR, notify=0x614000019060
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx The watcher list is empty and we have nothing to cancel. And we are wrong to rely on a cancellation callback to clean up.
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx The subchannel list is leaked.

D1127 13:24:35.779544302  117940 subchannel_list.cc:211]     [pick_first 0x60b000008f40] subchannel_list 0x608000029a20 UNREF 2->1 (selected_not_ready+switch_to_update)
D1127 13:24:35.779558407  117940 connectivity_state.cc:169]  SET: 0x60b000008f90 (null): TRANSIENT_FAILURE --> TRANSIENT_FAILURE [selected_not_ready+switch_to_update] error=0x611000045f00 {"created":"@1511817875.760229509","description":"GOAWAY received","file":"src/core/ext/transport/chttp2/transport/chttp2_transport.cc","file_line":1132,"http2_error":0,"raw_bytes":"Server shutdown"}
D1127 13:24:35.779577010  117940 pick_first.cc:369]          Pick First 0x60b000008f40 connectivity changed for subchannel 0x61600004ef80 (0 of 1), subchannel_list 0x608000029720: state=CONNECTING p->shutdown=0 sd->subchannel_list->shutting_down=0 error="No Error"
D1127 13:24:35.779585176  117940 connectivity_state.cc:169]  SET: 0x60b000008f90 (null): TRANSIENT_FAILURE --> CONNECTING [connecting_changed] error=(nil) "No Error"
D1127 13:24:35.779590747  117940 connectivity_state.cc:195]  NOTIFY: 0x60b000008f90 (null): 0x60b00000fbe8
D1127 13:24:35.779597687  117940 subchannel_list.cc:63]      [pick_first 0x60b000008f40] subchannel list 0x608000029720 index 0 of 1 (subchannel 0x61600004ef80, closure 0x60d0000173b0): requesting connectivity change notification
I1127 13:24:35.779604659  117940 subchannel.cc:525]          ABOUT TO PRINT THE WATCHER LIST
I1127 13:24:35.779609995  117940 subchannel.cc:529]          SUBCHANNEL 0x61600004ef80, WATCHER 0x60c000027dc0, NOTIFY 0x60d0000173b0
D1127 13:24:35.779615446  117940 connectivity_state.cc:118]  CONWATCH: 0x61600004f0a0 subchannel: from CONNECTING [cur=READY] notify=0x60c000027dd8

So the fix would be to make sure the last invocation to the connectivity callback will stop watching and unref for watching.
I haven't seen this leak after my fix is added.
But, I still have some timeout issue to fix. Still looking.

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from Reviewable

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +708 [None]                                                                               +9.07Ki  +0.2%
      +0.1%    +652 [Unmapped]                                                                           +9.04Ki  +0.2%
      +2.9%     +32 [None]                                                                                0  [ = ]
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.8%    +256 src/core/ext/filters/client_channel/client_channel.cc                                   +256  +1.8%
      +7.3%    +198 on_resolver_result_changed_locked                                                       +198  +7.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
   +43%    +204 src/core/ext/filters/client_channel/lb_policy.cc                                        +204   +43%
      [NEW]    +178 grpc_lb_policy_try_reresolve                                                            +178  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +0.6%     +96 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                           +96  +0.6%
      [NEW] +1.60Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.11]                  +1.60Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.9]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +2.6%     +10 [Unmapped]                                                                               +10  +2.6%
  +0.7%     +32 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +32  +0.7%
      +227%    +284 pf_shutdown_locked                                                                      +284  +227%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]
      +0.8%      +1 [Unmapped]                                                                                +1  +0.8%

 -------------- SHRINKING                                                                            --------------
  -4.1%    -240 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -240  -4.1%
     -16.6%    -344 rr_connectivity_changed_locked                                                          -344 -16.6%
      [DEL]    -300 shutdown_locked                                                                         -300  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.0]            -102  [DEL]

  +0.1% +1.03Ki TOTAL                                                                                +9.41Ki  +0.2%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +708 [None]                                                                               +9.07Ki  +0.2%
      +0.1%    +652 [Unmapped]                                                                           +9.04Ki  +0.2%
      +2.9%     +32 [None]                                                                                0  [ = ]
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.8%    +256 src/core/ext/filters/client_channel/client_channel.cc                                   +256  +1.8%
      +7.3%    +198 on_resolver_result_changed_locked                                                       +198  +7.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
   +43%    +204 src/core/ext/filters/client_channel/lb_policy.cc                                        +204   +43%
      [NEW]    +178 grpc_lb_policy_try_reresolve                                                            +178  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +0.6%     +96 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                           +96  +0.6%
      [NEW] +1.60Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.11]                  +1.60Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.9]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +2.6%     +10 [Unmapped]                                                                               +10  +2.6%
  +0.7%     +32 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +32  +0.7%
      +227%    +284 pf_shutdown_locked                                                                      +284  +227%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]
      +0.8%      +1 [Unmapped]                                                                                +1  +0.8%

 -------------- SHRINKING                                                                            --------------
  -4.1%    -240 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -240  -4.1%
     -16.6%    -344 rr_connectivity_changed_locked                                                          -344 -16.6%
      [DEL]    -300 shutdown_locked                                                                         -300  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.0]            -102  [DEL]

  +0.1% +1.03Ki TOTAL                                                                                +9.41Ki  +0.2%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

Do I need to worry about this or was I auto-added?

@ctiller You were auto-added. It seems that most of my LB PR's will auto-include you as an owner reviewer.

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +708 [None]                                                                               +9.14Ki  +0.2%
      +0.1%    +652 [Unmapped]                                                                           +9.11Ki  +0.2%
      +2.9%     +32 [None]                                                                                0  [ = ]
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.8%    +256 src/core/ext/filters/client_channel/client_channel.cc                                   +256  +1.8%
      +7.3%    +198 on_resolver_result_changed_locked                                                       +198  +7.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
   +43%    +204 src/core/ext/filters/client_channel/lb_policy.cc                                        +204   +43%
      [NEW]    +178 grpc_lb_policy_try_reresolve                                                            +178  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +0.6%     +96 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                           +96  +0.6%
      [NEW] +1.60Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.11]                  +1.60Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.9]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +2.6%     +10 [Unmapped]                                                                               +10  +2.6%
  +1.4%     +64 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +64  +1.4%
      +227%    +284 pf_shutdown_locked                                                                      +284  +227%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]
      +0.8%      +1 [Unmapped]                                                                                +1  +0.8%

 -------------- SHRINKING                                                                            --------------
  -4.1%    -240 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -240  -4.1%
     -16.6%    -344 rr_connectivity_changed_locked                                                          -344 -16.6%
      [DEL]    -300 shutdown_locked                                                                         -300  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.0]            -102  [DEL]

  +0.1% +1.06Ki TOTAL                                                                                +9.51Ki  +0.2%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +708 [None]                                                                               +9.14Ki  +0.2%
      +0.1%    +652 [Unmapped]                                                                           +9.11Ki  +0.2%
      +2.9%     +32 [None]                                                                                0  [ = ]
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.8%    +256 src/core/ext/filters/client_channel/client_channel.cc                                   +256  +1.8%
      +7.3%    +198 on_resolver_result_changed_locked                                                       +198  +7.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
   +43%    +204 src/core/ext/filters/client_channel/lb_policy.cc                                        +204   +43%
      [NEW]    +178 grpc_lb_policy_try_reresolve                                                            +178  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +0.6%     +96 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                           +96  +0.6%
      [NEW] +1.60Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.11]                  +1.60Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.9]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +2.6%     +10 [Unmapped]                                                                               +10  +2.6%
  +1.4%     +64 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +64  +1.4%
      +227%    +284 pf_shutdown_locked                                                                      +284  +227%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]
      +0.8%      +1 [Unmapped]                                                                                +1  +0.8%

 -------------- SHRINKING                                                                            --------------
  -4.1%    -240 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -240  -4.1%
     -16.6%    -344 rr_connectivity_changed_locked                                                          -344 -16.6%
      [DEL]    -300 shutdown_locked                                                                         -300  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.0]            -102  [DEL]

  +0.1% +1.06Ki TOTAL                                                                                +9.51Ki  +0.2%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

[microbenchmarks] No significant performance differences

[microbenchmarks] No significant performance differences

Reviewed 5 of 6 files at r23, 1 of 1 files at r24.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 405 at r24 (raw file):

        p->latest_pending_subchannel_list != nullptr) {
      p->selected = nullptr;
      grpc_lb_subchannel_data_stop_connectivity_watch(exec_ctx, sd);

Good analysis of the problem here!  I agree with your analysis of the problem.  And this does look like the right fix.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 437 at r24 (raw file):

        grpc_lb_subchannel_list_shutdown_and_unref(exec_ctx, p->subchannel_list,
                                                   "pf_selected_shutdown");
        p->subchannel_list = nullptr;

I don't think we should unref p->subchannel_list or set it to null here.  We don't really have any other cases where p->subchannel_list should be null, so introducing this will likely trigger a bug in some other part of the code that is not checking for this case.  In contrast, we do have cases where all of the subchannels in the list have been unreffed (i.e., sd->subchannel is null), so I think that's what we should do here.  We can do this by calling grpc_lb_subchannel_data_unref_subchannel().
(I don't think there's any benefit to unreffing p->subchannel_list here anyway, since that will happen when we either get the next update or we get shut down.)
Also, this discussion makes me realize that we need to schedule the reresolution closure in this case.  Otherwise, we will never recover from this failure -- the policy will always just be stuck with no selected subchannel and all of its subchannels shut down.
Another way to think about this is that this is essentially the same case as the one where we have not yet selected a subchannel and all of the subchannels that we've attempted have gone bad (see lines 542-551 below).  Both cases can be triggered by the same basic criteria, which is that all subchannels in p->subchannel_list have sd->subchannel set to null.  In the case on line 542, that's because we have tried all subchannels and they have all shut down before we could select one; in this case, it's because we selected one of them, which caused us to set sd->subchannel to null on all of the others (by calling destroy_unselected_subchannels_locked()), and then the one we selected shut down, so we did the same thing to it.

Comments from Reviewable

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

libgrpc.so

     VM SIZE                                                                                FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%    +748 [None]                                                                               +9.40Ki  +0.2%
      +0.1%    +692 [Unmapped]                                                                           +9.38Ki  +0.2%
      +2.9%     +32 [None]                                                                                0  [ = ]
     +10.0%      +8 glb_lb_policy_vtable                                                                      +8 +10.0%
     +10.0%      +8 pick_first_lb_policy_vtable                                                               +8 +10.0%
     +10.0%      +8 round_robin_lb_policy_vtable                                                              +8 +10.0%
  +1.8%    +256 src/core/ext/filters/client_channel/client_channel.cc                                   +256  +1.8%
      +7.3%    +198 on_resolver_result_changed_locked                                                       +198  +7.3%
      [NEW]    +157 request_reresolution_locked                                                             +157  [NEW]
   +43%    +204 src/core/ext/filters/client_channel/lb_policy.cc                                        +204   +43%
      [NEW]    +178 grpc_lb_policy_try_reresolve                                                            +178  [NEW]
       +15%     +20 [Unmapped]                                                                               +20   +15%
      [NEW]      +6 grpc_lb_policy_set_reresolve_closure_locked                                               +6  [NEW]
  +0.6%     +96 src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc                           +96  +0.6%
      [NEW] +1.60Ki rr_handover_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.11]                  +1.60Ki  [NEW]
      [NEW] +1.28Ki query_for_backends_locked(grpc_exec_ctx*, glb_lb_policy*) [clone .part.9]            +1.28Ki  [NEW]
      [NEW]    +834 build_lb_channel_args(grpc_exec_ctx*, grpc_lb_addresses const*, grpc_fake_resolver_r    +834  [NEW]
      [NEW]    +210 extract_backend_addresses_locked(grpc_exec_ctx*, grpc_lb_addresses const*) [clone .i    +210  [NEW]
      [NEW]    +143 glb_set_reresolve_closure_locked                                                        +143  [NEW]
      +2.6%     +10 [Unmapped]                                                                               +10  +2.6%
  +1.7%     +80 src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc                   +80  +1.7%
      +227%    +284 pf_shutdown_locked                                                                      +284  +227%
      [NEW]    +112 pf_set_reresolve_closure_locked                                                         +112  [NEW]
      [NEW]     +97 destroy_unselected_subchannels_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone      +97  [NEW]
      [NEW]     +80 start_picking_locked(grpc_exec_ctx*, pick_first_lb_policy*) [clone .isra.1]              +80  [NEW]
      +0.8%      +1 [Unmapped]                                                                                +1  +0.8%

 -------------- SHRINKING                                                                            --------------
  -4.1%    -240 src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc                -240  -4.1%
     -16.6%    -344 rr_connectivity_changed_locked                                                          -344 -16.6%
      [DEL]    -300 shutdown_locked                                                                         -300  [DEL]
      [DEL]    -102 start_picking_locked(grpc_exec_ctx*, round_robin_lb_policy*) [clone .isra.0]            -102  [DEL]

  +0.1% +1.12Ki TOTAL                                                                                +9.79Ki  +0.2%

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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

[trickle] No significant performance differences

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc, line 437 at r24 (raw file):
Previously, markdroth (Mark D. Roth) wrote…
I don't think we should unref p->subchannel_list or set it to null here.  We don't really have any other cases where p->subchannel_list should be null, so introducing this will likely trigger a bug in some other part of the code that is not checking for this case.  In contrast, we do have cases where all of the subchannels in the list have been unreffed (i.e., sd->subchannel is null), so I think that's what we should do here.  We can do this by calling grpc_lb_subchannel_data_unref_subchannel().
(I don't think there's any benefit to unreffing p->subchannel_list here anyway, since that will happen when we either get the next update or we get shut down.)