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

Fixed intermittent CPP sync server shutdown leak. #17151

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

arjunroy
Copy link
Contributor

@arjunroy arjunroy commented Nov 8, 2018

Specifically: if a request handling thread is in flight but scheduled
out when shutdown is called on the server, but it has already passed
the shutdown check, then when it resumes it will add a grpc_call to
the completion queue that is leaked. We fix this by explicitly freeing
such calls after all worker threads have shutdown.

To manifest the leak, run the end2end::ClientCancelsRequestStream
test repeatedly on the unpatched server implementation. About 0.5% of
the time, the leak will manifest.

Specifically: if a request handling thread is in flight but scheduled
out when shutdown is called on the server, but it has already passed
the shutdown check, then when it resumes it will add a grpc_call to
the completion queue that is leaked. We fix this by explicitly freeing
such calls after all worker threads have shutdown.

To manifest the leak, run the end2end::ClientCancelsRequestStream
test repeatedly on the unpatched server implementation. About 0.5% of
the time, the leak will manifest.
@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@soheilhy soheilhy added the release notes: no Indicates if PR should not be in release notes label Nov 8, 2018
@grpc-testing
Copy link

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

     VM SIZE                                                                                            FILE SIZE
 ++++++++++++++ GROWING                                                                              ++++++++++++++
  +0.1%     +65 src/cpp/server/server_cc.cc                                                              +65  +0.1%
      [NEW]    +200 void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >    +200  [NEW]
      +9.3%     +80 grpc::Server::ShutdownInternal                                                           +80  +9.3%
       +53%     +73 grpc::Server::SyncRequestThreadManager::Wait                                             +73   +53%
       +14%      +8 grpc::Server::SyncRequest::FinalizeResult                                                 +8   +14%

 -------------- SHRINKING                                                                            --------------
  -0.0%      -5 [None]                                                                                  -993  -0.0%

  +0.0%     +60 TOTAL                                                                                   -928  -0.0%



@grpc-testing
Copy link

[trickle] No significant performance differences

Copy link
Member

@yang-g yang-g left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

@grpc-testing
Copy link

Objective-C binary sizes
*****************STATIC******************
  New size                      Old size
 2,019,390      Total (=)      2,019,390

 No significant differences in binary sizes

***************FRAMEWORKS****************
  New size                      Old size
11,180,033      Total (>)     11,180,032

 No significant differences in binary sizes


@grpc-testing
Copy link

Corrupt JSON data (indicates timeout or crash): 
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.new: 10
    bm_call_create.BM_IsolatedFilter_ClientChannelFilter_NoOp_.counters.old: 10


[microbenchmarks] No significant performance differences

@arjunroy
Copy link
Contributor Author

arjunroy commented Nov 8, 2018

I signed it.

@yang-g
Copy link
Member

yang-g commented Nov 8, 2018

Thanks!

@yang-g yang-g merged commit ed6ea2c into grpc:master Nov 8, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Feb 7, 2019
@arjunroy arjunroy deleted the grpc_memleak_fix branch April 26, 2019 02:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants