issue #554: track and remove multiple make_tmp_path() calls.

ansible_mitogen/connection.py

@@ -33,7 +33,6 @@ import errno
 import logging
 import os
 import pprint
-import random
 import stat
 import sys
 import time
@@ -703,35 +702,6 @@ class Connection(ansible.plugins.connection.ConnectionBase):
         self._connect()
         return self.init_child_result['good_temp_dir']
 
-    def _generate_tmp_path(self):
-        return os.path.join(
-            self.get_good_temp_dir(),
-            'ansible_mitogen_action_%016x' % (
-                random.getrandbits(8*8),
-            )
-        )
-
-    def _make_tmp_path(self):
-        assert getattr(self._shell, 'tmpdir', None) is None
-        self._shell.tmpdir = self._generate_tmp_path()
-        LOG.debug('Temporary directory: %r', self._shell.tmpdir)
-        self.get_chain().call_no_reply(os.mkdir, self._shell.tmpdir)
-        return self._shell.tmpdir
-
-    def _reset_tmp_path(self):
-        """
-        Called by _mitogen_reset(); ask the remote context to delete any
-        temporary directory created for the action. CallChain is not used here
-        to ensure exception is logged by the context on failure, since the
-        CallChain itself is about to be destructed.
-        """
-        if getattr(self._shell, 'tmpdir', None) is not None:
-            self.context.call_no_reply(
-                ansible_mitogen.target.prune_tree,
-                self._shell.tmpdir,
-            )
-            self._shell.tmpdir = None
-
     def _connect(self):
         """
         Establish a connection to the master process's UNIX listener socket,
@@ -763,7 +733,6 @@ class Connection(ansible.plugins.connection.ConnectionBase):
         if not self.context:
             return
 
-        self._reset_tmp_path()
         self.chain.reset()
         self.parent.call_service(
             service_name='ansible_mitogen.services.ContextService',

ansible_mitogen/mixins.py

@@ -30,6 +30,7 @@ from __future__ import absolute_import
 import logging
 import os
 import pwd
+import random
 import traceback
 
 try:
@@ -173,26 +174,48 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase):
         """
         assert False, "_is_pipelining_enabled() should never be called."
 
+    def _generate_tmp_path(self):
+        return os.path.join(
+            self._connection.get_good_temp_dir(),
+            'ansible_mitogen_action_%016x' % (
+                random.getrandbits(8*8),
+            )
+        )
+
+    def _generate_tmp_path(self):
+        return os.path.join(
+            self._connection.get_good_temp_dir(),
+            'ansible_mitogen_action_%016x' % (
+                random.getrandbits(8*8),
+            )
+        )
+
     def _make_tmp_path(self, remote_user=None):
         """
-        Return the directory created by the Connection instance during
-        connection.
+        Create a temporary subdirectory as a child of the temporary directory
+        managed by the remote interpreter.
         """
         LOG.debug('_make_tmp_path(remote_user=%r)', remote_user)
-        return self._connection._make_tmp_path()
+        path = self._generate_tmp_path()
+        LOG.debug('Temporary directory: %r', path)
+        self._connection.get_chain().call_no_reply(os.mkdir, path)
+        self._connection._shell.tmpdir = path
+        return path
 
     def _remove_tmp_path(self, tmp_path):
         """
-        Stub out the base implementation's invocation of rm -rf, replacing it
-        with nothing, as the persistent interpreter automatically cleans up
-        after itself without introducing roundtrips.
+        Replace the base implementation's invocation of rm -rf, replacing it
+        with a pipelined call to :func:`ansible_mitogen.target.prune_tree`.
         """
-        # The actual removal is pipelined by Connection.close().
         LOG.debug('_remove_tmp_path(%r)', tmp_path)
-        # Upstream _remove_tmp_path resets shell.tmpdir here, however
-        # connection.py uses that as the sole location of the temporary
-        # directory, if one exists.
-        # self._connection._shell.tmpdir = None
+        if tmp_path is None and ansible.__version__ > '2.6':
+            tmp_path = self._connection._shell.tmpdir  # 06f73ad578d
+        if tmp_path is not None:
+            self._connection.get_chain().call_no_reply(
+                ansible_mitogen.target.prune_tree,
+                tmp_path,
+            )
+        self._connection._shell.tmpdir = None
 
     def _transfer_data(self, remote_path, data):
         """

docs/changelog.rst

@@ -41,6 +41,10 @@ Fixes
   TTY-related `ioctl()` calls on Windows Subsystem for Linux 2016 Anniversary
   Update.
 
+* `#554 <https://github.com/dw/mitogen/issues/554>`_: third party Ansible
+  action plug-ins that invoked :func:`_make_tmp_path` repeatedly could trigger
+  an assertion failure.
+
 * `ffae0355 <https://github.com/dw/mitogen/commit/ffae0355>`_: needless
   information was removed from the documentation and installation procedure.
 
@@ -77,6 +81,7 @@ bug reports, testing, features and fixes in this release contributed by
 `Matt Layman <https://github.com/mblayman>`_,
 `Percy Grunwald <https://github.com/percygrunwald>`_,
 `Petr Enkov <https://github.com/enkov>`_,
+`Tony Finch <https://github.com/fanf2>`_,
 `@elbunda <https://github.com/elbunda>`_, and
 `@zyphermonkey <https://github.com/zyphermonkey>`_.
 

tests/ansible/integration/action/all.yml

@@ -2,6 +2,7 @@
 - include: fixup_perms2__copy.yml
 - include: low_level_execute_command.yml
 - include: make_tmp_path.yml
+- include: make_tmp_path__double.yml
 - include: remote_expand_user.yml
 - include: remote_file_exists.yml
 - include: remove_tmp_path.yml

tests/ansible/integration/action/make_tmp_path__double.yml

@@ -0,0 +1,20 @@
+# issue #554: double calls to make_tmp_path() fail with assertion error. Ensure
+# they succeed and are cleaned up correctly.
+
+- hosts: target
+  tasks:
+  - mitogen_action_script:
+      script: |
+        result['t1'] = self._make_tmp_path()
+        result['t2'] = self._make_tmp_path()
+        assert result['t1'] != result['t2']
+        assert self._remote_file_exists(result['t1'])
+        assert self._remote_file_exists(result['t2'])
+        self._remove_tmp_path(result['t1'])
+        self._remove_tmp_path(result['t2'])
+    register: out
+
+  - mitogen_action_script:
+      script: |
+        assert not self._remote_file_exists("{{ out.t1 }}")
+        assert not self._remote_file_exists("{{ out.t2 }}")