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

Fix connection keep-alive for request-response. #1700

Merged
merged 3 commits into from
Aug 13, 2020
Merged

Fix connection keep-alive for request-response. #1700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
23d05c0
Fix connection keep-alive for request-response.
Aug 13, 2020
51cabe6
Merge branch 'master' into request-response-keepalive
romanb Aug 13, 2020
ac9f06f
Merge branch 'master' into request-response-keepalive
romanb Aug 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions protocols/request-response/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# 0.2.0 // unreleased

- Fixed connection keep-alive, permitting connections to close due
to inactivity.
- Added `RequestResponse::throttled` to wrap the behaviour into one that
enforces limits on inbound and outbound requests per peer. The limits
have to be known upfront by all nodes.
Expand Down
2 changes: 1 addition & 1 deletion protocols/request-response/src/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ where
self.outbound.shrink_to_fit();
}

if self.inbound.is_empty() {
if self.inbound.is_empty() && self.keep_alive.is_yes() {
Copy link
Member

Choose a reason for hiding this comment

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

Paraphrasing this to make sure I am on the same page: Without the additional && self.keep_alive.is_yes() this condition would be triggered on each poll if the inbound queue is empty and thus the self.keep_alive timeout would be refreshed each time leaving it no time to expire. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct.

// No new inbound or outbound requests. However, we may just have
// started the latest inbound or outbound upgrade(s), so make sure
// the keep-alive timeout is preceded by the substream timeout.
Expand Down