Skip to content

Conversation

@azrle
Copy link
Contributor

@azrle azrle commented Nov 30, 2020

Hi @leehambley

This is the PR that aims to partially solve #483.
Sorry for the delay.

The change will do a simple polling (process(0)) to check if the connection is still alive before we reuse it. If the connection is closed by remote, the polling will get <IOError: closed stream> which will be caught and consider the connection is already closed. It partially fixes #483 since when the keepalive check fails and remote host closes the connection, the connection won't be reused.

However, it will not completely fix the issue because the keepalive packet should be responded by ssh-kit/net-ssh but it doesn't. This could be fixed by introducing another thread that keep pooling connections in pool so that net-ssh will reply to keepalive packets. But I'm reluctant to do so since it would introduce new threads to maintain and complexity such as interval settings. I would like to fix the closed connection reuse issue first and leave the complete fix to others if it's needed.

By the way, you can check out 48d6ff3 and run the test then it will reproduce the #483.

@leehambley
Copy link
Member

Hi @azrle - excellent work. Very gratifying to see a test for this with some appropriate SSH config. Lamentable that we have a sleep in the tests, but I don't see a reasonable alternative.

@leehambley leehambley merged commit 692a5f4 into capistrano:master Jan 5, 2021
@mattbrictson mattbrictson added the 🐛 Bug Fix Fixes a bug label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐛 Bug Fix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

connection pool session keepalive

3 participants