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

Close streams with Reset instead of Close when closing streaming connections #22802

Merged
merged 1 commit into from
Mar 11, 2016
Merged

Close streams with Reset instead of Close when closing streaming connections #22802

merged 1 commit into from
Mar 11, 2016

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Mar 10, 2016

This ensures that the call to close the underlying streaming connection will execute immediately,
instead of waiting for all streams to gracefully shut down.

Ref #19014

@ncdc ncdc mentioned this pull request Mar 10, 2016
Closed
@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 10, 2016
@ncdc ncdc added this to the v1.2 milestone Mar 10, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 10, 2016

GCE e2e build/test passed for commit a0535451aebc236ce6d5d02425ea1e1a30d5325e.

@ncdc
Copy link
Member Author

ncdc commented Mar 10, 2016

cc @yujuhong FYI this, along with #22804 should hopefully resolve the goroutine leaks related to spdystream in #19014

@ncdc ncdc mentioned this pull request Mar 10, 2016
Merged
@yujuhong
Copy link
Contributor

cc @yujuhong FYI this, along with #22804 should hopefully resolve the goroutine leaks related to spdystream in #19014

@ncdc, thanks for quick debugging and fixes!

@ncdc
Copy link
Member Author

ncdc commented Mar 10, 2016

@liggitt care to review?

@ncdc
Copy link
Member Author

ncdc commented Mar 10, 2016

cc @kubernetes/rh-cluster-infra

@lavalamp
Copy link
Member

Is this correctness or just an optimization? If the former, should I be asking for a test? (if not, LGTM)

@ncdc
Copy link
Member Author

ncdc commented Mar 11, 2016

I should be able to do a test for this

On Thursday, March 10, 2016, Daniel Smith notifications@github.com wrote:

Is this correctness or just an optimization? If the former, should I be
asking for a test? (if not, LGTM)

—
Reply to this email directly or view it on GitHub
#22802 (comment)
.

Close streams with Reset instead of Close when closing streaming conn…
01b33ec 
…ections

This ensures that the call to close the underlying streaming connection will execute immediately,
instead of waiting for all streams to gracefully shut down.
@ncdc ncdc force-pushed the httpstream-reset-streams-when-closing-connection branch from a053545 to 01b33ec Compare March 11, 2016 16:28
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 11, 2016
@ncdc
Copy link
Member Author

ncdc commented Mar 11, 2016

@lavalamp now with unit test (holy cow that was difficult to write!)

@ncdc
Copy link
Member Author

ncdc commented Mar 11, 2016

@lavalamp if you omit the change to connection.go, this new test will fail.

@k8s-bot
Copy link

k8s-bot commented Mar 11, 2016

GCE e2e build/test passed for commit 01b33ec.

@ncdc
Copy link
Member Author

ncdc commented Mar 11, 2016

@k8s-bot unit test this issue: #22856

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2016
@lavalamp
Copy link
Member

LGTM, thanks for the test!!

@ncdc
Copy link
Member Author

ncdc commented Mar 11, 2016

@bgrant0607 @yujuhong what priority is appropriate for this?

@ncdc ncdc mentioned this pull request Mar 11, 2016
Closed
85 tasks
@lavalamp lavalamp added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Mar 11, 2016
@yujuhong
Copy link
Contributor

P1 is right. Thanks @ncdc @lavalamp

@bgrant0607
Copy link
Member

Manually merging. @k8s-oncall

bgrant0607 added a commit that referenced this pull request Mar 11, 2016
@bgrant0607
Merge pull request #22802 from ncdc/httpstream-reset-streams-when-clo…
d514a65 
…sing-connection

Close streams with Reset instead of Close when closing streaming connections
@bgrant0607 bgrant0607 merged commit d514a65 into kubernetes:master Mar 11, 2016
@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 11, 2016
eparis pushed a commit to eparis/kubernetes that referenced this pull request Mar 11, 2016
@bgrant0607 @eparis
Merge pull request kubernetes#22802 from ncdc/httpstream-reset-stream…
279ad2f 
…s-when-closing-connection

Close streams with Reset instead of Close when closing streaming connections
@eparis eparis mentioned this pull request Mar 11, 2016
Merged
@eparis
Copy link
Contributor

eparis commented Mar 11, 2016

This PR is included in #22874 which is slated to be included in the release-1.2 branch.
Please verify that the cherry-pick in that PR looks correct.

@eparis eparis removed cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Mar 11, 2016
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@bgrant0607 @eparis
Merge pull request kubernetes#22802 from ncdc/httpstream-reset-stream…
305623e 
…s-when-closing-connection

Close streams with Reset instead of Close when closing streaming connections
@ncdc ncdc deleted the httpstream-reset-streams-when-closing-connection branch February 13, 2017 17:24
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@bgrant0607 @eparis
Merge pull request kubernetes#22802 from ncdc/httpstream-reset-stream…
23e6d83 
…s-when-closing-connection

Close streams with Reset instead of Close when closing streaming connections
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@ncdc @k8s-github-robot @k8s-bot @yujuhong @lavalamp @bgrant0607 @eparis @k8s-teamcity-mesosphere @googlebot