Fix bug in Magic when destructor called too early

[koreno]

Potentially magic_open may fail, and when python attempts to gc the Magic instance it fails with:

Exception ignored in: <function Magic.__del__ at 0x7f595ba06430>
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/magic.py", line 129, in __del__
    if self.cookie and magic_close:
AttributeError: 'Magic' object has no attribute 'cookie'

[ahupp]

Oh, I just got an issue about this today and wasn't sure what was going on. What is the failure in magic_open?

[koreno]

Oh, I just got an issue about this today and wasn't sure what was going on. What is the failure in magic_open?

Actually I haven't had a chance to dig into that yet, but that's the only thing that would explain .cookie not existing...

[koreno]

I found out the failure wasn't even in magic_open - it was an API change that our jira module wasn't ready for:

Exception ignored in: <function Magic.__del__ at 0x7f9af0aae310>
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/magic.py", line 129, in __del__
    if self.cookie and magic_close:
AttributeError: 'Magic' object has no attribute 'cookie'
TypeError: __init__() got an unexpected keyword argument 'flags'
> /usr/local/lib/python3.8/site-packages/jira/client.py(2575)_try_magic()
   2573         else:
   2574             try:
-> 2575                 _magic = magic.Magic(flags=magic.MAGIC_MIME_TYPE)
   2576 
   2577                 def cleanup(x):

[ahupp]

On closer look, this issue still doesn't make sense to me. If magic_open throws, then that exception will propagate out of the Magic constructor and the object is never successfully created, and then del is not called.

Passing bad args to __init__ should also not cause this either for similar reasons.

A similar issue was reported in https://github.com/ahupp/python-magic/issues/221.

Just to make sure I know what we're running, can you share a copy of your magic.py here?

[koreno]

__del__ is called regardless of whether __init__ passed. When __init__ fails the object is already created, simply not initialized:

In [43]: class Foo(): 
    ...:     def __init__(self): 
    ...:         pass 
    ...:     def __del__(self): 
    ...:         print("del", self) 
    ...:          
    ...:                                                                                                                                                                                       

In [44]: Foo(1)                                                                                                                                                                                
del <__main__.Foo object at 0x7fb5734d6850>
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-44-e75a32c75a19> in <module>
----> 1 Foo(1)

TypeError: __init__() takes 1 positional argument but 2 were given

As I said above this actually did not fail in magic_open. I ultimately tracked the issue to a python environment issue with python-jira:

    def _try_magic(self):
        try:
            import magic
            import weakref
        except ImportError:
            self._magic = None
        else:
            try:
                _magic = magic.Magic(flags=magic.MAGIC_MIME_TYPE)

                def cleanup(x):
                    _magic.close()

                self._magic_weakref = weakref.ref(self, cleanup)
                self._magic = _magic
            except TypeError:
                self._magic = None
            except AttributeError:
                self._magic = None

Apparently python-jira expects the filemagic python package, which takes a flags parameter, but for some reason our environment had your python-magic package, which expects different parameters.

[koreno]

(This means that my patch wouldn't really solve the problem for my case...)

[ahupp]

@koreno I figured out my confusion about constructors throwing and destructors: in C++ the destructor is not run if the constructor throws. I'd tested this in the python REPL and it also appeared to behave that way, but it seems to just be a REPL quirk that defers destruction until the next statement:

https://gist.github.com/ahupp/7924bcebed149e176d4a0ed57afdce4b

[ahupp]

In any event, thanks again for the patch.