Question about signal handling and __cinit__

[Toan Vuong]

Hello,

I'm troubleshooting a crash in our application, and I ran into an issue as follows:

import signal

from scipy.spatial import ConvexHull
import numpy as np

def alarm_handler(*args, **kwargs):
    raise Exception('ALARM')

signal.signal(signal.SIGALRM, alarm_handler)
signal.alarm(1)
rng = np.random.default_rng()
points = rng.random((30, 2))
hull = ConvexHull(points)

The crash happens when the alarm signal is signaled while __cinit__ is happening for this object (It doesn't seem to crash if the SIGALRM is raised after __cinit__ is done). My question is, what's the expected behavior when SIGALRM gets raised when execution is in the middle of a __cinit__? It looks to me like the object is not yet fully allocated, but then subsequently the object gets deallocated, and crashes during deallocation. For context, if I keep everything in the python world (i.e. use __init__, __del__, and def close()), there's no crash. But use the corresponding Cython constructs (__cinit__, __dealloc__, cdef close()) and it'll crash.

Thanks,

Toan

[da-woods]

So as far as I can tell from the documentation:

if you install a Python signal handler (using the signal module) the contents of `alarm_handler` get executed after the next bytecode is interpreted. For a block of Cython code (e.g. `__cinit__`) that means I typically wouldn't expect the signal handler to get executed while the Cython code is running. However, Cython code can easily and unexpectedly trigger Python code to be run (for example if you decref something and that calls a `__del__` function). In such cases I'd typically expect Cython to handle it

fairly gracefully because it will expect anything that executes Python bytecode to be able to raise an exception.

However, it's definitely possible than an exception from an unexpected point in `__cinit__` could leave the object in an invalid state that the authors haven't dealt with correctly.

I think we'd be interested in a bug report if you could reproduce it with a really simple (e.g. almost empty) cdef class, but not necessarily if it was just with a specific scipy class where the logic with file-descriptors looks fairly complex.

--

---
You received this message because you are subscribed to the Google Groups "cython-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cython-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/cython-users/4625989e-70d1-4f8f-aa1c-bb3c2da3ff8dn%40googlegroups.com.

[Oscar Benjamin]

It is not immediately clear what would cause the crash here but you
should be clearer about what "crash" means (segfault? exception?
something else?).

I can definitely see many problems with this code though. In general I
would say that there is just way too much happening in this __cinit__
method and also too much in __dealloc__:
https://github.com/scipy/scipy/blob/v1.10.0/scipy/_lib/messagestream.pyx#L21

Messing around with files, calling into Python code, catching
exceptions etc just should not really happen in either __cinit__ or
__dealloc__.

The purpose of __cinit__ is just to leave the object with its C-level
state ready for __dealloc__ to be called safely. That does not mean
that __cinit__ necessarily needs to initialise anything to its
intended value but just that any invariants expected by __dealloc__
must be satisfied after __cinit__ exits. Usually that just means
setting everything to NULL which is actually done implicitly before
__cinit__ is called anyway. Then if __dealloc__ checks for NULL
everything is fine.

It is possible that code in __cinit__ might exit from raising an
exception in which case __dealloc__ would be called immediately so any
place where an exception could be raised is a place where all
invariants must be satisfied. If potentially calling into any Python
code then there is a possibility for an exception to be raised
(including from a signal) so the invariants must be satisfied at that
point. Basically you need to think that __dealloc__ is just something
that can happen at almost any time.

The __dealloc__ method for this class calls self.close() which in turn checks
if not self._removed
This is a poor choice of variable because the default initialisation
used by Cython will be to set _removed to False. It would be better to
invert the meaning of this boolean variable so that False is the safe
value. There are ways that __cinit__ might exit with an exception
without initialising this value including the possibility that
self._filename might be uninitialised (e.g. mkstemp or os.remove could
fail or a signal could be raised during those calls).

Also it is a mistake to try to use RAII like this in Python even when
done from Cython. It would be better for this object to be a context
manager i.e. move all the code from __cinit__ and __dealloc__ to
__enter__ and __exit__.

--
Oscar

[Toan Vuong]

Thank you for your reply. Apologies for the omission earlier -- the error is a segfault, and the segfault seems to happen in `__dealloc__` (I assume it's probably trying to call `self.close()` on a partially constructed object that's only been `__cinit__`, or something of that nature).

Unfortunately I don't own the code and don't fully understand all of its uses -- I tried to take a look and am not sure if it's possible (without a lot of refactor) to change it into a context manager type of usage as opposed to doing the close currently in `__dealloc__` or `__del__`.. The suggestions make sense and I'll link this thread to the ticket I opened with scipy.