Skip to content

Commit

Permalink
Remove ansible_mitogen Connection.close() workaround
Browse files Browse the repository at this point in the history
Refs #925 #969

I'm not 100% confident that merely removing this is the full fix,
without substituting something else. I am sure keeping it would be
the greater of two evils. __del__() should be avoided on general
principal, and it's associated with multiple intermittant CI
failures, plus multiple user reported issues.
  • Loading branch information
moreati committed Nov 4, 2022
1 parent 03acf40 commit 0af2ce8
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 9 deletions.
27 changes: 18 additions & 9 deletions ansible_mitogen/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ class Connection(ansible.plugins.connection.ConnectionBase):
login_context = None

#: Only sudo, su, and doas are supported for now.
# Ansible ConnectionBase attribute, removed in Ansible >= 2.8
become_methods = ['sudo', 'su', 'doas']

#: Dict containing init_child() return value as recorded at startup by
Expand Down Expand Up @@ -521,15 +522,6 @@ class Connection(ansible.plugins.connection.ConnectionBase):
# set by `_get_task_vars()` for interpreter discovery
_action = None

def __del__(self):
"""
Ansible cannot be trusted to always call close() e.g. the synchronize
action constructs a local connection like this. So provide a destructor
in the hopes of catching these cases.
"""
# https://github.com/dw/mitogen/issues/140
self.close()

def on_action_run(self, task_vars, delegate_to_hostname, loader_basedir):
"""
Invoked by ActionModuleMixin to indicate a new task is about to start
Expand Down Expand Up @@ -684,6 +676,9 @@ def get_binding(self):

@property
def connected(self):
"""
Ansible connection plugin property. Used by ansible-connection command.
"""
return self.context is not None

def _spec_from_via(self, proxied_inventory_name, via_spec):
Expand Down Expand Up @@ -842,7 +837,11 @@ def _connect(self):
the _connect_*() service calls defined above to cause the master
process to establish the real connection on our behalf, or return a
reference to the existing one.
Ansible connection plugin method.
"""
# In some Ansible connection plugins this method returns self.
# However nothing I've found uses it, it's not even assigned.
if self.connected:
return

Expand Down Expand Up @@ -880,6 +879,8 @@ def close(self):
Arrange for the mitogen.master.Router running in the worker to
gracefully shut down, and wait for shutdown to complete. Safe to call
multiple times.
Ansible connection plugin method.
"""
self._put_connection()
if self.binding:
Expand All @@ -896,6 +897,8 @@ def reset(self):
any local state we hold for the connection, returns the Connection to
the 'disconnected' state, and informs ContextService the connection is
bad somehow, and should be shut down and discarded.
Ansible connection plugin method.
"""
if self._play_context.remote_addr is None:
# <2.5.6 incorrectly populate PlayContext for reset_connection
Expand Down Expand Up @@ -1002,6 +1005,8 @@ def exec_command(self, cmd, in_data='', sudoable=True, mitogen_chdir=None):
Data to supply on ``stdin`` of the process.
:returns:
(return code, stdout bytes, stderr bytes)
Ansible connection plugin method.
"""
emulate_tty = (not in_data and sudoable)
rc, stdout, stderr = self.get_chain().call(
Expand All @@ -1027,6 +1032,8 @@ def fetch_file(self, in_path, out_path):
Remote filesystem path to read.
:param str out_path:
Local filesystem path to write.
Ansible connection plugin method.
"""
self._connect()
ansible_mitogen.target.transfer_file(
Expand Down Expand Up @@ -1076,6 +1083,8 @@ def put_file(self, in_path, out_path):
Local filesystem path to read.
:param str out_path:
Remote filesystem path to write.
Ansible connection plugin method.
"""
try:
st = os.stat(in_path)
Expand Down
5 changes: 5 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ v0.3.4.dev0

* :gh:issue:`929` Support Ansible 6 and ansible-core 2.13
* :gh:issue:`832` Fix runtime error when using the ansible.builtin.dnf module multiple times
* :gh:issue:`925` :class:`ansible_mitogen.connection.Connection` no longer tries to close the
connection on destruction. This is expected to reduce cases of `mitogen.core.Error: An attempt
was made to enqueue a message with a Broker that has already exitted`. However it may result in
resource leaks.


v0.3.3 (2022-06-03)
-------------------
Expand Down

0 comments on commit 0af2ce8

Please sign in to comment.