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

Fix "Could not connect to x: not connected" errors #211

Merged
merged 1 commit into from
May 8, 2023
Merged

Fix "Could not connect to x: not connected" errors #211

merged 1 commit into from
May 8, 2023

Conversation

foutrelis
Copy link
Contributor

Tweak SSHConnectionManager.Connect() to return a new connection if the previous one is not connected. (This occurs with expired connections.)

Maybe fixes: #56

Tweak SSHConnectionManager.Connect() to return a new connection if the
previous one is not connected. (This occurs with expired connections.)

Maybe fixes: #56
@foutrelis foutrelis requested a review from czerwonk as a code owner May 2, 2023 09:43
@czerwonk czerwonk merged commit 727b229 into czerwonk:main May 8, 2023
@foutrelis foutrelis deleted the fix-not-connected-errors branch May 8, 2023 15:50
@foutrelis
Copy link
Contributor Author

Hi @czerwonk. Thank you for merging this fix. Please note that there is one more issue regarding connection handling where SSHConnection.close() is called on expired/closed connections (for example, when stopping the exporter with expired connections present).

This remaining issue is less annoying (as it doesn't affect the exporter's normal functioning), but if wrapping c.done <- struct{}{} in a c.isConnected() condition seems like a decent approach, I can submit a PR for it.

When I first encountered the original issue my first thought was to ensure SSHConnectionManager.connections doesn't contain expired/closed connections. But for some reason I don't readily recall, I didn't go that route and instead added the "connect if not connected" fix seen in this PR. Perhaps if my first thought isn't too hard to implement, it would fix the shutdown issue as well?

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.

Exporter did not recover from connection loss
2 participants