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.

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

Cancel max idle timer when shutting down the server channel #14501

Merged
merged 1 commit into from
Feb 26, 2018

Conversation

y-zeng
Copy link
Contributor

@y-zeng y-zeng commented Feb 23, 2018

The max idle timer was not shutdown properly when the server channel is being shutdown. This caused a reference of each channel stack to be held until the max idle timer times out.

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                      FILE SIZE
 ++++++++++++++ GROWING                                        ++++++++++++++
  +0.0%      +8 [None]                                            +136  +0.0%
  +1.1%     +32 src/core/ext/filters/max_age/max_age_filter.cc     +32  +1.1%
       +25%     +40 channel_connectivity_changed                       +40   +25%

  +0.0%     +40 TOTAL                                             +168  +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

@abaldeva
Copy link

Fix looks good on my end. Thx!

I see that at least one of the automated build has failed but can't see the details. Any idea when this fix would become part of an official release? Would it be part of 1.9.2 or go to 1.10.0?

Thanks.

@y-zeng
Copy link
Contributor Author

y-zeng commented Feb 23, 2018

Looks like it's a testing issue. The test could not fetch the abseil submodule. I'll try it again.

@y-zeng
Copy link
Contributor Author

y-zeng commented Feb 23, 2018

I'll try to catch the 1.10.0 train.

@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                      FILE SIZE
 ++++++++++++++ GROWING                                        ++++++++++++++
  +0.0%      +8 [None]                                            +128  +0.0%
  +1.1%     +32 src/core/ext/filters/max_age/max_age_filter.cc     +32  +1.1%
       +25%     +40 channel_connectivity_changed                       +40   +25%

  +0.0%     +40 TOTAL                                             +160  +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

mehrdada commented Feb 24, 2018

Please merge this on 1.10.x first (edit the target branch on top of GitHub PR) and upmerge instead master-then-backport. It should be easy since everything on the 1.10.x branch is already upmerged, and it'll be a clean upmerge. Thanks!

UPDATE: I guess it needs a rebase locally and a push -f as the GitHub UI does not do that :(

@mehrdada mehrdada changed the base branch from master to v1.10.x February 26, 2018 18:18
@mehrdada mehrdada changed the base branch from v1.10.x to master February 26, 2018 18:18
@mehrdada mehrdada changed the base branch from master to v1.10.x February 26, 2018 18:22
@mehrdada mehrdada merged commit 7f734dd into grpc:v1.10.x Feb 26, 2018
@grpc-testing
Copy link

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

libgrpc.so

     VM SIZE                                                      FILE SIZE
 ++++++++++++++ GROWING                                        ++++++++++++++
  +0.0%      +8 [None]                                            +136  +0.0%
  +1.1%     +32 src/core/ext/filters/max_age/max_age_filter.cc     +32  +1.1%
       +25%     +40 channel_connectivity_changed                       +40   +25%

  +0.0%     +40 TOTAL                                             +168  +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

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants