Skip to content

fix or slow down windows socket leak#13604

Merged
jtattermusch merged 1 commit intogrpc:masterfrom
yayaboring:master
Dec 13, 2017
Merged

fix or slow down windows socket leak#13604
jtattermusch merged 1 commit intogrpc:masterfrom
yayaboring:master

Conversation

@yayaboring
Copy link
Copy Markdown
Contributor

@yayaboring yayaboring commented Dec 4, 2017

related issue
#13519
#13102

@thelinuxfoundation
Copy link
Copy Markdown

thelinuxfoundation commented Dec 4, 2017

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,
The Linux Foundation CLA GitHub bot

@yayaboring yayaboring mentioned this pull request Dec 4, 2017
Closed
@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 5, 2017 

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

libgrpc.so

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

  [ = ]       0        0  [ = ]


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

libgrpc++.so

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

  [ = ]       0        0  [ = ]

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 5, 2017 

[trickle] No significant performance differences

@grpc-testing
Copy link
Copy Markdown

grpc-testing commented Dec 5, 2017 

[microbenchmarks] No significant performance differences

@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 12, 2017

@yang-g, can you take a look?

@jtattermusch jtattermusch requested a review from yang-g December 12, 2017 00:31
@yayaboring
Copy link
Copy Markdown
Contributor Author

yayaboring commented Dec 13, 2017 •
edited
Loading

similar code on server side.

grpc/src/core/lib/iomgr/tcp_server_windows.cc

Line 330 in c5df7fd

wsa_success = WSAGetOverlappedResult(sock, &info->overlapped,

@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 13, 2017

Known test failures:
#13122

jtattermusch
jtattermusch approved these changes Dec 13, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing!

@jtattermusch jtattermusch merged commit de651a6 into grpc:master Dec 13, 2017
@jtattermusch jtattermusch mentioned this pull request Dec 13, 2017
Closed
@jtattermusch
Copy link
Copy Markdown
Contributor

jtattermusch commented Dec 13, 2017

It might be a good idea to backport this into 1.8.1.

This was referenced Dec 13, 2017
Closed
Merged
@smflorentino
Copy link
Copy Markdown

smflorentino commented Dec 13, 2017

+1, thanks for fixing & backporting - our services are being impacted by this because gRPC attempts to connect to IPv6 first and fails (our service only exposes an IPv4 endpoint) - so every channel creation leaks a socket. When is 1.8.1 going to be released?

jtattermusch added a commit that referenced this pull request Dec 13, 2017
@jtattermusch
Merge pull request #13744 from jtattermusch/backport_13604
04fb1c0 
Backport #13604 to v1.8.x
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nicolasnoble nicolasnoble Awaiting requested review from nicolasnoble

@yang-g yang-g Awaiting requested review from yang-g

1 more reviewer

@jtattermusch jtattermusch jtattermusch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nicolasnoble nicolasnoble

@yang-g yang-g

Labels

platform/Windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@yayaboring @thelinuxfoundation @grpc-testing @jtattermusch @smflorentino @nicolasnoble @yang-g @kokoro-team