Skip to content

avoid connection leaks (#80532) #80787

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

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from
Open

avoid connection leaks (#80532) #80787

wants to merge 2 commits into from

Conversation

blami
Copy link

@blami blami commented May 13, 2023

SUMMARY

In case TaskExecutor needs to open a new connection it should attempt to properly dispose existing one if any.

Fixes #80532

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

core

In case TaskExecutor needs to open a new connection it should attempt to
properly dispose existing one if any.

fixes: ansible#80532
@ansibot ansibot added affects_2.16 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. labels May 13, 2023
except AttributeError:
pass
except Exception as e:
display.debug(u"error closing connection: %s" % to_text(e))
Copy link
Member

Choose a reason for hiding this comment

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

aside from missing tests and a changelog ... this might not be a good approach as other connections are reused (even if not for the current task) in the play and this would require the costly re-initialization of them.

I know the winrm connection does somethings differently but a general reaper at the end of the run might be a better solution, though many connections might accumulate during the run. Another issue is that some setups reuse the connections created by Ansible (mostly ssh) , so it would have to be a toggle or a per connection property.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for valuable comment, I made this PR more like hotfix to get some guidance how to fix it properly - I am sorry if it should been discussion in Issue first... It seemed to me that task_executor always closes connection at the end of .run() anyway and wasn't aware that connection can be retained over self._connection.close() call there.

I think general reaper at the end might not be good option either as default WinRM quota is quite low (I think its 30 connections per user) so if playbook has a task with more than 30 loop items it will blew before reaper kicks in...

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label May 16, 2023
@bcoca
Copy link
Member

bcoca commented May 16, 2023

cc @jborean93 @nitzmahone

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 24, 2023
@nitzmahone nitzmahone self-assigned this Feb 21, 2024
@nitzmahone nitzmahone added the P3 Priority 3 - Approved, No Time Limitation label Feb 21, 2024
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 6, 2024
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 15, 2024
@ansibot ansibot added the stale_pr This PR has not been pushed to for more than one year. label Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.16 bug This issue/PR relates to a bug. has_issue new_contributor This PR is the first contribution by a new community member. P3 Priority 3 - Approved, No Time Limitation stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_pr This PR has not been pushed to for more than one year.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WinRM winrshost processes hanging after playbook run finishes
4 participants