Skip to content

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

Merged
mehrdada merged 1 commit intogrpc:v1.10.xfrom
y-zeng:fix_max_idle
Feb 26, 2018
Merged

Cancel max idle timer when shutting down the server channel#14501
mehrdada merged 1 commit intogrpc:v1.10.xfrom
y-zeng:fix_max_idle

Conversation

@y-zeng
Copy link
Copy Markdown
Contributor

@y-zeng y-zeng commented Feb 23, 2018 •
edited
Loading

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.

@y-zeng y-zeng requested a review from sreecha February 23, 2018 04:40
@y-zeng y-zeng mentioned this pull request Feb 23, 2018
Closed
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 23, 2018 

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

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
Copy Markdown

grpc-testing commented Feb 23, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 23, 2018 

[microbenchmarks] No significant performance differences

@abaldeva
Copy link
Copy Markdown

abaldeva commented Feb 23, 2018

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
Copy Markdown
Contributor Author

y-zeng commented Feb 23, 2018 •
edited
Loading

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

@y-zeng
Copy link
Copy Markdown
Contributor Author

y-zeng commented Feb 23, 2018

I'll try to catch the 1.10.0 train.

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 23, 2018 

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

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
Copy Markdown

grpc-testing commented Feb 23, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 23, 2018 

[microbenchmarks] No significant performance differences

@mehrdada
Copy link
Copy Markdown
Contributor

mehrdada commented Feb 24, 2018 •
edited
Loading

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
Copy Markdown

grpc-testing commented Feb 26, 2018 

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

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  [ = ]

@mehrdada mehrdada mentioned this pull request Feb 26, 2018
Merged
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 26, 2018 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Feb 26, 2018 

[microbenchmarks] No significant performance differences

@apolcyn apolcyn mentioned this pull request Mar 1, 2018
Closed
@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

1 more reviewer

@sreecha sreecha sreecha approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@y-zeng @grpc-testing @abaldeva @mehrdada @sreecha @kokoro-team