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

idle sweeper: Don't close connections with pending calls #712

Merged
merged 2 commits into from
Jul 30, 2018

Conversation

prashantv
Copy link
Contributor

As documented in #701, we currently attempt closing connections even
if they have pending calls. This can cause issues if the calls is stuck
as the connection enters start-close, so it'll reject new incoming
calls, but it doesn't close, so the remote side keeps sending calls over
the same connection.

Fixes #701.

@prashantv prashantv requested a review from witriew July 27, 2018 21:35
@prashantv prashantv force-pushed the idle_with_pending branch from a3357f5 to 6178642 Compare July 27, 2018 21:38
As documented in #701, we currently attempt closing connections even
if they have pending calls. This can cause issues if the calls is stuck
as the connection enters start-close, so it'll reject new incoming
calls, but it doesn't close, so the remote side keeps sending calls over
the same connection.

Fixes #701.
@prashantv prashantv force-pushed the idle_with_pending branch from 6178642 to 595bdfc Compare July 27, 2018 21:39
@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #712 into dev will increase coverage by 0.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #712      +/-   ##
==========================================
+ Coverage   87.39%   87.73%   +0.33%     
==========================================
  Files          40       40              
  Lines        4047     4052       +5     
==========================================
+ Hits         3537     3555      +18     
+ Misses        386      377       -9     
+ Partials      124      120       -4
Impacted Files Coverage Δ
connection.go 85.71% <100%> (+1.07%) ⬆️
idle_sweep.go 96.07% <100%> (+0.24%) ⬆️
introspection.go 92.38% <0%> (+0.95%) ⬆️
preinit_connection.go 94.16% <0%> (+1.45%) ⬆️
outbound.go 90.22% <0%> (+2.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfb0917...8145bcd. Read the comment docs.


listener := newPeerStatusListener()
serverOpts := testutils.NewOpts().
AddLogFilter("Skip closing idle Connection as it has pending calls.", 1).
Copy link
Contributor

Choose a reason for hiding this comment

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

Was looking over this and I initially thought that it would check and verify for this log message. It'd probably be great if we could do that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a TODO

Copy link
Contributor

@witriew witriew left a comment

Choose a reason for hiding this comment

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

Looks good.

@prashantv prashantv merged commit fe44733 into dev Jul 30, 2018
@prashantv prashantv deleted the idle_with_pending branch July 30, 2018 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calls can still be pending for a connection that's idle
2 participants