Backport #13594 to v1.9.x by y-zeng · Pull Request #14122 · grpc/grpc

y-zeng · 2018-01-22T21:02:32Z

The previous implementation is faulty:
Assume we have one active call.

When it ends, decrease_call_count will be invoked, gpr_atm_full_fetch_add decreases call_count from 1 to 0.
At this time, a new call arrives, increase_call_count is called, gpr_atm_full_fetch_add sees that call_count is 0. It then cancels the timer.
decrease_call_count then continues its operation, and uses grpc_timer_init to set up the timer.

After these operations, there is still an active call, but the max_idle timer is set. At the time this remaining call ends, it will cause the crash reported in #12257.

The new implementation does not cancel max_idle_timer (unless the channel is shutting down). Instead, it uses idle_state to record if max_idle_timer is still valid. If the timer callback is called when the max_idle_timer is valid (i.e. idle_state is MAX_IDLE_STATE_TIMER_SET), the channel will be closed due to idleness, otherwise the channel won't be changed.

y-zeng · 2018-01-22T21:05:10Z

cc @mehrdada

grpc-testing · 2018-01-22T21:05:35Z

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

libgrpc.so

     VM SIZE                                                      FILE SIZE
 ++++++++++++++ GROWING                                        ++++++++++++++
   +17%    +448 src/core/ext/filters/max_age/max_age_filter.cc    +448   +17%
      [NEW]    +394 max_idle_timer_cb                                 +394  [NEW]
      +195%    +160 decrease_call_count                               +160  +195%
      +286%    +103 increase_call_count                               +103  +286%
      +2.1%     +16 init_channel_elem                                  +16  +2.1%

 -+-+-+-+-+-+-+ MIXED                                          +-+-+-+-+-+-+-
  -0.0%      -8 [None]                                         +1.25Ki  +0.0%

  +0.0%    +440 TOTAL                                          +1.69Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-22T21:17:33Z

[trickle] No significant performance differences

grpc-testing · 2018-01-22T21:56:58Z

Corrupt JSON data (indicates timeout or crash): 
    bm_fullstack_unary_ping_pong.BM_UnaryPingPong_InProcessCHTTP2_NoOpMutator_NoOpMutator__64_0.counters.new: 1


[microbenchmarks] No significant performance differences

mehrdada · 2018-01-22T22:15:09Z

@y-zeng if this PR is not yet merged on master, why not make this PR the primary one and get it upmerged alongside the rest of 1.9.x instead of having two PRs on master and 1.9.x?

y-zeng · 2018-01-23T00:13:33Z

@mehrdada We're work on this PR and using the the other one to test changes against master. I'll close the #13594 after this one is merged.

mehrdada · 2018-01-23T00:17:28Z

@y-zeng Feel free to do whatever it is easier for you. I just thought upmerge is better because I imagined it would be easier for you, but whatever you feel is fine with me. Makes no difference.

grpc-testing · 2018-01-23T02:23:15Z

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

libgrpc.so

     VM SIZE                                                      FILE SIZE
 ++++++++++++++ GROWING                                        ++++++++++++++
   +17%    +448 src/core/ext/filters/max_age/max_age_filter.cc    +448   +17%
      [NEW]    +388 max_idle_timer_cb                                 +388  [NEW]
      +195%    +160 decrease_call_count                               +160  +195%
      +286%    +103 increase_call_count                               +103  +286%
      +2.1%     +16 init_channel_elem                                  +16  +2.1%
      +2.1%      +3 [Unmapped]                                          +3  +2.1%

 -+-+-+-+-+-+-+ MIXED                                          +-+-+-+-+-+-+-
  -0.0%      -8 [None]                                         +1.32Ki  +0.0%

  +0.0%    +440 TOTAL                                          +1.76Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-23T02:33:47Z

[trickle] No significant performance differences

grpc-testing · 2018-01-23T03:15:07Z

[microbenchmarks] No significant performance differences

mehrdada · 2018-01-23T21:46:56Z

Friendly ping: are we ready to merge this?

sreecha · 2018-01-23T21:49:18Z

src/core/ext/filters/max_age/max_age_filter.cc

+     | MAX_IDLE_STATE_SEEN_ENTER_IDLE | set, invalid   | idle    |
+     +--------------------------------+----------------+---------+
+
+     MAX_IDLE_STATE_INIT: The initial and final state of 'idle_state'. The


Thanks for the very detailed comments!!

y-zeng · 2018-01-23T22:57:43Z

@sreecha Thanks for the review!
@mehrdada Some tests were not finished due to kokoro issues, I'll restart them and merge this PR ASAP.

grpc-testing · 2018-01-23T23:03:14Z

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

libgrpc.so

     VM SIZE                                                      FILE SIZE
 ++++++++++++++ GROWING                                        ++++++++++++++
   +17%    +448 src/core/ext/filters/max_age/max_age_filter.cc    +448   +17%
      [NEW]    +388 max_idle_timer_cb                                 +388  [NEW]
      +195%    +160 decrease_call_count                               +160  +195%
      +286%    +103 increase_call_count                               +103  +286%
      +2.1%     +16 init_channel_elem                                  +16  +2.1%
      +2.1%      +3 [Unmapped]                                          +3  +2.1%

 -+-+-+-+-+-+-+ MIXED                                          +-+-+-+-+-+-+-
  -0.0%      -8 [None]                                         +1.32Ki  +0.0%

  +0.0%    +440 TOTAL                                          +1.76Ki  +0.0%


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

grpc-testing · 2018-01-23T23:13:16Z

[trickle] No significant performance differences

grpc-testing · 2018-01-23T23:55:59Z

[microbenchmarks] No significant performance differences

y-zeng · 2018-01-24T01:12:52Z

Known issues:
Asan C++: #13148
Basic Tests C++ Linux: #13477, #14153

y-zeng added 5 commits January 22, 2018 12:54

Add necessary retries for atomic state changes

f46028f

Try again when idle_state is unexpected

c4431e4

Fix asan issues

d1d605e

Document idle_state transistions

b22befd

Fix merge issues

6a15e4f

y-zeng added the priority/P0/RELEASE BLOCKER label Jan 22, 2018

y-zeng assigned sreecha Jan 22, 2018

y-zeng requested a review from sreecha January 22, 2018 21:02

mehrdada mentioned this pull request Jan 22, 2018

1.9.0-pre1 is now 1.9.0-pre2 #14114

Merged

y-zeng mentioned this pull request Jan 23, 2018

Refactor max idle filter #13594

Closed

Change if to switch, add more descriptions

a041e0f

sreecha approved these changes Jan 23, 2018

View reviewed changes

sreecha reviewed Jan 23, 2018

View reviewed changes

y-zeng added the kokoro:force-run label Jan 23, 2018

kokoro-team removed the kokoro:force-run label Jan 23, 2018

y-zeng merged commit 02384e3 into grpc:v1.9.x Jan 24, 2018

y-zeng added this to the 2016/05/21 Milestone milestone Jan 24, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

lock bot unassigned sreecha Jan 20, 2019

Conversation

y-zeng commented Jan 22, 2018

Uh oh!

y-zeng commented Jan 22, 2018

Uh oh!

grpc-testing commented Jan 22, 2018

Uh oh!

grpc-testing commented Jan 22, 2018

Uh oh!

grpc-testing commented Jan 22, 2018

Uh oh!

mehrdada commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

y-zeng commented Jan 23, 2018

Uh oh!

mehrdada commented Jan 23, 2018

Uh oh!

grpc-testing commented Jan 23, 2018

Uh oh!

grpc-testing commented Jan 23, 2018

Uh oh!

grpc-testing commented Jan 23, 2018

Uh oh!

mehrdada commented Jan 23, 2018

Uh oh!

sreecha Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

y-zeng commented Jan 23, 2018

Uh oh!

grpc-testing commented Jan 23, 2018

Uh oh!

grpc-testing commented Jan 23, 2018

Uh oh!

grpc-testing commented Jan 23, 2018

Uh oh!

y-zeng commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mehrdada commented Jan 22, 2018 •

edited

Loading