[go: up one dir, main page]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Backport #13594 to v1.9.x #14122

Merged
merged 6 commits into from
Jan 24, 2018
Merged

Backport #13594 to v1.9.x #14122

merged 6 commits into from
Jan 24, 2018

Conversation

y-zeng
Copy link
Contributor
@y-zeng y-zeng commented Jan 22, 2018

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 y-zeng requested a review from sreecha January 22, 2018 21:02
@y-zeng
Copy link
Contributor Author
y-zeng commented Jan 22, 2018

cc @mehrdada

@grpc-testing
Copy link 
****************************************************************

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
Copy link 
[trickle] No significant performance differences

@mehrdada mehrdada mentioned this pull request Jan 22, 2018
Merged
@grpc-testing
Copy link 
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
Copy link
Member
mehrdada commented Jan 22, 2018 •
edited
Loading

@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
Copy link
Contributor Author
y-zeng commented Jan 23, 2018

@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
Copy link
Member

@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.

@y-zeng y-zeng mentioned this pull request Jan 23, 2018
Closed
@grpc-testing
Copy link 
****************************************************************

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
Copy link 
[trickle] No significant performance differences

@grpc-testing
Copy link 
[microbenchmarks] No significant performance differences

@mehrdada
Copy link
Member

Friendly ping: are we ready to merge this?

sreecha
sreecha reviewed Jan 23, 2018
View reviewed changes
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the very detailed comments!!

@y-zeng
Copy link
Contributor Author
y-zeng commented Jan 23, 2018

@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
Copy link 
****************************************************************

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
Copy link 
[trickle] No significant performance differences

@grpc-testing
Copy link 
[microbenchmarks] No significant performance differences

@y-zeng
Copy link
Contributor Author
y-zeng commented Jan 24, 2018

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

@y-zeng y-zeng merged commit 02384e3 into grpc:v1.9.x Jan 24, 2018
@y-zeng y-zeng added this to the 2016/05/21 Milestone milestone Jan 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
@lock lock bot unassigned sreecha Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sreecha sreecha sreecha approved these changes

Assignees
No one assigned
Labels
priority/P0/RELEASE BLOCKER
Projects
None yet
Milestone
2016/05/21 Milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@y-zeng @grpc-testing @mehrdada @sreecha @kokoro-team