Python destructor refactoring and exception safety

src/bindings/python/flux/core/handle.py

@@ -29,14 +29,6 @@ class Flux(Wrapper):
   """
 
     def __init__(self, url=ffi.NULL, flags=0, handle=None):
-        self.external = True
-        self.handle = None
-        self.closer = raw.flux_close
-
-        if handle is None:
-            handle = raw.flux_open(url, flags)
-            self.external = False
-
         super(self.__class__, self).__init__(
             ffi, lib,
             handle=handle,
@@ -45,24 +37,11 @@ def __init__(self, url=ffi.NULL, flags=0, handle=None):
             prefixes=[
                 'flux_',
                 'FLUX_',
-            ], )
-
-    def close(self):
-        if not self.external and self.handle is not None:
-            self.closer(self.handle)
-            self.handle = None
+            ],
+            destructor = raw.flux_close,)
 
-    def __del__(self):
-        self.close()
-
-    def __enter__(self):
-        """Allow this to be used as a context manager"""
-        return self
-
-    def __exit__(self, type_arg, value, tb):
-        """Allow this to be used as a context manager"""
-        self.close()
-        return False
+        if handle is None:
+            self.handle = raw.flux_open(url, flags)
 
     def log(self, level, fstring):
         """Log to the flux logging facility
@@ -101,8 +80,8 @@ def rpc_send(self, topic,
                  nodeid=flux.FLUX_NODEID_ANY,
                  flags=0):
         """ Create and send an RPC in one step """
-        r = RPC(self, topic, payload, nodeid, flags)
-        return r.get()
+        with RPC(self, topic, payload, nodeid, flags) as r:
+            return r.get()
 
     def rpc_create(self, topic,
                    payload=None,

src/bindings/python/flux/kvs.py

@@ -4,6 +4,7 @@
 import json
 import collections
 import errno
+import sys
 
 class KVSWrapper(Wrapper):
   # This empty class accepts new methods, preventing accidental overloading
@@ -90,26 +91,23 @@ def watch_once(flux_handle, key):
 class KVSDir(WrapperPimpl, collections.MutableMapping):
     class InnerWrapper(Wrapper):
         def __init__(self, flux_handle=None, path='.', handle=None):
-            self.destroyer = _raw.kvsdir_destroy
-            self.handle = None
-            if flux_handle is None and handle is None:  # pragma: no cover
-                raise ValueError(
-                    "flux_handle must be a valid Flux object or handle must be a valid kvsdir cdata pointer")
-            if handle is None:
-                d = ffi.new("kvsdir_t *[1]")
-                _raw.kvs_get_dir(flux_handle, d, path)
-                handle = d[0]
-
             super(self.__class__, self).__init__(ffi, lib,
                                                  handle=handle,
                                                  match=ffi.typeof('kvsdir_t *'),
                                                  prefixes=[
                                                      'kvsdir_',
-                                                 ], )
+                                                 ],
+                                                 destructor=_raw.kvsdir_destroy)
 
-        def __del__(self):
-            if self.handle is not None:
-                self.destroyer(self.handle)
+            if flux_handle is None and handle is None:  # pragma: no cover
+                raise ValueError(
+                    "flux_handle must be a valid Flux object or handle must be a valid kvsdir cdata pointer")
+            if handle is None:
+                d = ffi.new("kvsdir_t *[1]")
+                _raw.kvs_get_dir(flux_handle, d, path)
+                self.handle = d[0]
+                if self.handle is None or self.handle == ffi.NULL:
+                    raise EnvironmentError("No such file or directory")
 
     def __init__(self, flux_handle=None, path='.', handle=None):
         self.fh = flux_handle

src/bindings/python/flux/message.py

@@ -15,31 +15,24 @@ class InnerWrapper(Wrapper):
         def __init__(self,
                      type_id=flux.FLUX_MSGTYPE_REQUEST,
                      handle=None,
-                     destruct=False):
-            self.destruct = destruct
-            if handle is None:
-                self.external = False
-                handle = raw.flux_msg_create(type_id)
-            else:
-                self.external = True
+                     destruct=False,):
             super(self.__class__, self).__init__(
                 ffi, lib,
                 handle=handle,
                 match=ffi.typeof(lib.flux_msg_create).result,
                 prefixes=[
                     'flux_msg_',
                     'FLUX_MSG',
-                ], )
+                ],
+                destructor=raw.flux_msg_destroy if destruct else None,)
+            if handle is None:
+                self.handle = raw.flux_msg_create(type_id)
 
-        def __del__(self):
-            if ((not self.external or self.destruct) and
-                self.handle is not None and self.handle != ffi.NULL):
-                raw.flux_msg_destroy(self.handle)
 
     def __init__(self,
                  type_id=flux.FLUX_MSGTYPE_REQUEST,
                  handle=None,
-                 destruct=False):
+                 destruct=False,):
         self.pimpl = self.InnerWrapper(type_id, handle, destruct)
 
     @property

src/bindings/python/flux/rpc.py

@@ -1,5 +1,5 @@
 from flux.wrapper import Wrapper, WrapperPimpl
-from flux._core import ffi, lib
+from flux.core.inner import ffi, lib, raw
 import flux
 import json
 
@@ -13,10 +13,17 @@ def __init__(self,
                  nodeid=flux.FLUX_NODEID_ANY,
                  flags=0,
                  handle=None):
-      self.external = False
-      if handle is not None:
-        self.external = True
-      else:
+      # hold a reference for destructor ordering
+      self.fh = flux_handle
+      super(self.__class__, self).__init__(ffi, lib,
+                                       handle=handle,
+                                       match=ffi.typeof(lib.flux_rpc).result,
+                                       prefixes=[
+                                         'flux_rpc_',
+                                         ],
+                                       destructor=raw.flux_rpc_destroy,
+                                       )
+      if handle is None:
         if isinstance(flux_handle, Wrapper):
           flux_handle = flux_handle.handle
 
@@ -27,22 +34,9 @@ def __init__(self,
 
         if isinstance(nodeid, basestring):
           # String version is multiple nodeids
-          handle = lib.flux_rpc_multi(flux_handle, topic, payload, nodeid, flags)
+          self.handle = lib.flux_rpc_multi(flux_handle, topic, payload, nodeid, flags)
         else:
-          handle = lib.flux_rpc(flux_handle, topic, payload, nodeid, flags)
-
-
-      super(self.__class__, self).__init__(ffi, lib, 
-                                       handle=handle,
-                                       match=ffi.typeof(lib.flux_rpc).result,
-                                       prefixes=[
-                                         'flux_rpc_',
-                                         ],
-                                       )
-    def __del__(self):
-      pass
-      # if not self.external and self.handle is not None:
-      #   lib.flux_rpc_destroy(self.handle)
+          self.handle = lib.flux_rpc(flux_handle, topic, payload, nodeid, flags)
 
   def __init__(self,
                flux_handle,

src/bindings/python/flux/sec.py

@@ -3,14 +3,11 @@
 
 class Sec(Wrapper):
   def __init__(self, handle=None):
-    if handle is None:
-      self.external = False
-      handle = lib.flux_sec_create(type_id)
-    else:
-      self.external = True
     super(self.__class__, self).__init__(ffi, lib, 
                                      handle=handle,
                                      match=ffi.typeof(lib.flux_sec_create).result,
                                      prefixes=['flux_sec_'],
+                                     destructor=lib.flux_sec_destroy,
                                      )
-
+    if handle is None:
+      self.handle = lib.flux_sec_create(type_id)

src/bindings/python/flux/wrapper.py

@@ -53,8 +53,6 @@ def __call__(self, *args):
 # A do-nothing class for checking for composition-based wrappers that offer a
 # handle attribute
 class WrapperBase(object):
-    __slots__ = ['_handle']
-
     def __init__(self):
         self._handle = None
 
@@ -68,8 +66,6 @@ def handle(self, handle):
 
 
 class WrapperPimpl(WrapperBase):
-    __slots__ = ['pimpl']
-
     @property
     def handle(self):
         return self.pimpl.handle
@@ -78,21 +74,28 @@ def handle(self):
     def handle(self, handle):
         self.pimpl.handle = handle
 
+    def __enter__(self):
+        return self
+
+    def __exit__(self, type_arg, value, tb):
+        self.pimpl.__exit__(type_arg, value, tb)
 
 class WrongNumArguments(ValueError):
-    def __init__(self, name, signature, ftype, arguments):
+    def __init__(self, name, signature, ftype, arguments, htype):
         message = """
 The wrong number of arguments has been passed to wrapped C function:
 Expected {expected} arguments, received {received}
 Name: {name}
 C signature: {c_type}
 Arguments: {arguments}
+Handle type: {htype}
           """.format(
             name=name,
             c_type=signature,
             arguments=arguments,
             expected=len(ftype.args),
-            received=len(arguments), )
+            received=len(arguments),
+            htype=htype)
         super(WrongNumArguments, self).__init__(message)
 
 
@@ -159,7 +162,8 @@ def __call__(self, calling_object, *args_in):
             if len(self.function_type.args) != len(args):
                 raise WrongNumArguments(self.name,
                                         self.ffi.getctype(self.function_type),
-                                        self.function_type, args)
+                                        self.function_type, args,
+                                        self.ffi.typeof(calling_object.handle) if calling_object.handle else "None")
             for i in self.arg_trans:
                 if args[i] is None:
                     args[i] = calling_object.ffi.NULL
@@ -186,41 +190,36 @@ def __call__(self, calling_object, *args_in):
         return result
 
 
+
 class Wrapper(WrapperBase):
     """
     Forms a wrapper around an interface that dynamically searches for undefined
     names, and can detect and pass a handle argument of specified type when it
     is found in the signature of an un-specified target function.
     """
 
-    __slots__ = [
-        'ffi',
-        'lib',
-        #'handle', # inherited from WrapperBase as _handle, wrapped by prop
-        'match',
-        'filter_match',
-        'prefixes',
-        'NULL',
-    ]
-
     def __init__(self, ffi, lib,
                  handle=None,
                  match=None,
                  filter_match=True,
-                 prefixes=[], ):
+                 prefixes=[],
+                 destructor=None, ):
+        self._handle = None
         super(Wrapper, self).__init__()
 
         self.ffi = ffi
         self.lib = lib
-        self.handle = handle
+        self._handle = handle
         self.match = match
         self.filter_match = filter_match
         self.prefixes = prefixes
+        self.destructor = destructor
 
         self.NULL = ffi.NULL
+        self.initialized = True
 
     def check_handle(self, name, t):
-        if self.match is not None and self.handle is not None:
+        if self.match is not None and self._handle is not None:
             if t.kind == 'function' and t.args[
                 0
             ] == self.match:  #first argument is of handle type
@@ -240,24 +239,25 @@ def check_wrap(self, fun, name):
 
     def __getattr__(self, name):
         fun = None
+        llib = self.__getattribute__("lib")
         if re.match('__.*__', name):
           # This is a python internal name, skip it
           raise AttributeError
         #try it bare
         try:
-            fun = getattr(self.lib, name)
+            fun = getattr(llib, name)
         except AttributeError:
             for prefix in self.prefixes:
                 try:
                     #wrap it
-                    fun = getattr(self.lib, prefix + name)
+                    fun = getattr(llib, prefix + name)
                     break
                 except AttributeError:
                     pass
         if fun is None:
             # Return a proxy class to generate a good error on call
             setattr(self.__class__, name, ErrorPrinter(name, self.prefixes))
-            return getattr(self, name)
+            return self.__getattribute__(name)
 
         if not callable(fun): # pragma: no cover
           return fun
@@ -266,4 +266,37 @@ def __getattr__(self, name):
         new_method = MethodType(new_fun, None, self.__class__)
         # Store the wrapper function into the class to prevent a second lookup
         setattr(self.__class__, name, new_method)
-        return getattr(self, name)
+        return self.__getattribute__(name)
+
+    def __clear(self):
+        # avoid recursion
+        if not self.initialized:
+            return
+        h = self._handle
+        if h is None:
+            return
+        if self.destructor is None:
+            return
+        self.destructor(h)
+        # ensure we don't double destruct
+        self._handle = None
+
+    @property
+    def handle(self):
+        return self._handle
+
+    @handle.setter
+    def handle(self, h):
+        """ Override handle setter to clean up old handle if requested """
+        if self._handle is not None:
+            self.__clear()
+        self._handle = h
+
+    def __del__(self):
+        self.__clear()
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, type_arg, value, tb):
+        self.__clear()