Rethink SEGFAULT protection

[woutdenolf]

Regarding the multiprocessing problem we have arrived at a place where the idea of minimal changes is more difficult than re-thinking the entire thing imo.

So I'm starting again 4 years ago when this was introduced: #848.

class H5NodeProxy(object):

    def raw_keys(self):
        # !!!!!! THIS CAN SEGFAULT when the node is h5py.File !!!!!!
        for node in self.children:
            yield node.name


class H5FileProxy(H5NodeProxy):
    def raw_keys(self):
        if isinstance(self.file, h5py.File):
            # The root node is a native h5py.File object
            file_path = self.file.filename
            data_path = self.name
            return HDF5Utils.safe_hdf5_group_keys(file_path, data_path=data_path)
        else:
            return super().raw_keys()

Note that the SEGFAULTs happen when looking at a file that is being written to (so online).

So in summary: when are getting the keys of an h5py.File instance we can get a SEGFAULT and we want to protect ourselves from it.

Protecting against SEGFAULTs can only be done by using a subprocesses. We cannot pass the actual h5py.File object so we need to pass the file_path and data_path (which is always going to be "/" in this case).

The reason when we don't expect safe_hdf5_group_keys to raise an error is that we already have self.file which is an opened file object. So any failure would be most likely due to a SEGFAULT. I think this is where the bad design starts to creep in because this assumption makes the implementation of safe_hdf5_group_keys unclear.

So if we forget about what calls this and just look a a function that protect against crashing the main process but other wide behaves like normal h5py (you get exceptions when HDF5 raises execptions):

def safe_hdf5_group_keys(file_path, data_path="/"):
     """Return a list of keys for the HDF5 group or [] if the process crashes (segfault)."""
    return _run_with_segfault_protection(
        _get_hdf5_group_keys,
        file_path,
        data_path,
        default=[],
    )


def _get_hdf5_group_keys(file_path, data_path):
    import h5py
    with h5py.File(file_path, "r") as f:
        return list(f[data_path].keys())


def _run_with_segfault_protection(func, *args, default=None, timeout=5, **kwargs):
    """
    Run `func(*args, **kwargs)` in a subprocess to isolate crashes.

    - Returns result if subprocess succeeds
    - Returns `default` if subprocess crashes or hangs
    - Falls back to direct execution if multiprocessing unavailable
    """
    queue = None
    p = None

    try:
        import multiprocessing

        ctx = multiprocessing.get_context("spawn")
        queue = ctx.Queue(maxsize=1)

        p = ctx.Process(
            target=_subprocess_worker,
            args=(queue, func, args, kwargs),
        )
        p.start()

        p.join(timeout)

        # Subprocess still alive? force kill
        if p.is_alive():
            try:
                p.terminate()  # graceful attempt
            except Exception:
                pass
            try:
                p.kill()  # forceful attempt
            except Exception:
                pass
            p.join()
            return default

        # If subprocess crashed
        if p.exitcode != 0:
            return default

        try:
            status, payload = queue.get(timeout=1)
        except Exception:
            return default

        if status == "ok":
            return payload
        else:
            raise payload

    except Exception:
        # Fallback: multiprocessing unavailable or not working
        return func(*args, **kwargs)

    finally:
        # Safe cleanup
        if queue is not None:
            try:
                queue.close()
            except Exception:
                pass
        if p is not None:
            try:
                p.close()
            except Exception:
                pass


def _subprocess_worker(queue, func, args, kwargs):
    try:
        result = func(*args, **kwargs)
        queue.put(("ok", result))
    except Exception as exc:
        # We treat Python exceptions as normal results
        queue.put(("error", exc))

The big difference with the current implementation is that we make a different between a normal python exception and a SEGFAULT.

[woutdenolf]

Removing multiprocessing is an equal amount of effort. The _run_with_segfault_protection would look like this:

import sys, pickle, subprocess, tempfile, os

def _run_with_segfault_protection(func, *args, default=None, timeout=5, **kwargs):
    """
    Run a Python function in a separate subprocess to isolate crashes or hangs.

    Returns the result of func(*args, **kwargs) if successful.
    If the subprocess crashes, segfaults, hangs, or times out, returns `default`.
    """

    with tempfile.NamedTemporaryFile(delete=False) as tf:
        tf_path = tf.name

    # Pickle function + args to another temp file
    with tempfile.NamedTemporaryFile(delete=False) as pf:
        pickle.dump((func, args, kwargs), pf)
        pf_path = pf.name

    python_code = (
        f"import pickle;"
        f"func, args, kwargs = pickle.load(open(r'{pf_path}', 'rb'));"
        f"res = func(*args, **kwargs);"
        f"pickle.dump(res, open(r'{tf_path}', 'wb'))"
    )

    try:
        proc = subprocess.Popen([sys.executable, "-c", python_code],
                                stdout=subprocess.PIPE,
                                stderr=subprocess.PIPE)
        try:
            proc.communicate(timeout=timeout)
        except subprocess.TimeoutExpired:
            proc.kill()
            proc.communicate()
            return default

        if proc.returncode != 0:
            return default

        # Load result from temp file
        try:
            with open(tf_path, "rb") as f:
                return pickle.load(f)
        except Exception:
            return default

    finally:
        for path in (tf_path, pf_path):
            try:
                os.unlink(path)
            except Exception:
                pass

[woutdenolf]

Is subprocess.Popen([sys.executable, "-c", python_code] enough for frozen binaries, no idea.

[vasole]

Just to try to address your questions.

For frozen binaries, no need to protect anything to access the HDF5 files. We can avoid the calls to multiprocessing and the proof that works is that frozen binaries without including the multiprocessing module were the usual approach.

The subprocess module is already used by PyMca in frozen binaries but only to start frozen binaries or programs. There is no issue at all using it but there is no sys.executable:

You are the father of the safe_hdf5_group_keys and it works nicely at beamlines. I do not consider worth the effort to change it just to make it available to frozen binaries irrespective of multiprocessing being supported or not. Perhaps you consider is almost zero effort and you are ready to do it. It is your call.

To me, the argument to keep using the approach found by @sergey-yaroslavtsev (kudos to him), it is the one described in the comments of the PR. Since multiprocessing is a standard module, even if we do our best not to depend on it, we cannot escape from other standard library trying to use it in some critical function. It is perhaps unlikely because windows is a different world (no fork, just spawn) but the possibility is there.

[vasole]

@woutdenolf

Your last suggestion concerning sys.executable made me think about alternatives I studied in the past.

Should we give up shipping a frozen executable and instead provide an installer with the option to install the code at a existing python interpreter or as standalone with shortcuts in OS specific places?

I recall in the early days users had to run a .bat file downloading and installing everything in an existing python installation -cybersecurity was something different by then-. Even that was too hard for them but the frozen binary was the natural approach for them.

[woutdenolf]

For frozen binaries, no need to protect anything to access the HDF5 files.

When we try to access the live HDF5 file on a Windows computer at the beamline you might crash the program (segfault) by refreshing when you do not protect against HDF5 root node access segfaults. This is what safe_hdf5_group_keys is for. Of course the protect needs to be there of every OS. But I mention Windows because those are most likely frozen binaries.

I do not consider worth the effort to change it

For me it is worth the effort because the semi-support of multiprocessing makes everything very difficult. We need to handle multiprocessing being there or not (this is handled by capturing exceptions but then we retry in the main process which could cause a segfault anyway). We need to handle multiprocessing being there but basically not working without an exception that indicates that (this is fixed by using freeze_support() everywhere). We need to distinguish between a segfault and a normal error (not handled). And god knows what we forgot, didn't consider or don't know. On top of that we have multiple layers of "protection" which adds to the confusion:

            try:
                if sys.platform.startswith("win") and \
                   getattr(sys, 'frozen', False):
                    _logger.debug("Frozen executable. Using standard approach")
                    return self.file[data_path].keys()
                else:
                    from PyMca5.PyMcaIO import HDF5Utils
                    return HDF5Utils.safe_hdf5_group_keys(file_path,
                                                      data_path=data_path)
            except Exception:
                _logger.debug("Using standard approach")
                return self.file[data_path].keys()

That's why I strongly think it is the moment to remove all the capturing, re-raise, freeze_support and design from scratch.

My position still remains that we only use multiprocessing if we can make it work in all situations, which is currently not the case obviously.

made me think about alternatives I studied in the past

I think whatever we come up with, the users should not be impacted. For example we have PyMcaMain.exe so I assume there should be a way to create a python.exe with the same CLI as the normal python.

[woutdenolf]

@vasole Can you remind me why we cannot or don't want to support multiprocessing with multiprocessing.freeze_support() in all cases (meaning no need to capture any exceptions related to multiprocessing)?

[vasole]

@vasole Can you remind me why we cannot or don't want to support multiprocessing with multiprocessing.freeze_support() in all cases (meaning no need to capture any exceptions related to multiprocessing)?

• We are already supporting multiprocessing with freeze_support() in all frozen modules
• Optional dependencies must never compromise the stability/usability of the code. Multiprocessing is an optional dependency for PyMca. It has always been optional. It is not the same case for subprocess.
• The try/except is a fallback net (no crash due to an optional dependency, builds with previous freezing chain and straightforward debugging).

[woutdenolf]

So then let me rephrase the question: why is multiprocessing optional? It must be that we do not support multiprocessing in all cases then.

[vasole]

So then let me rephrase the question: why is multiprocessing optional?

Because it is used in a particular situation of access to HDF5 files (accessing HDF5 files while being written) and it has been giving troubles to generate frozen binaries since day one. Keeping it optional allows to bypass freezing problems by simply excluding the module from the freezing procedure.

The current situation is satisfactory to me. Frozen modules try to add the support of multiprocessing but they will not crash if there is an issue with it.

[sergey-yaroslavtsev]

Sorry if I am missing the point but I would like to summarize a bit. Feel free to edit this comment.

PyMca as it is now:

1. It is not clear in the process of frozen binaries if HDF5 is read using MP or not (because there is complicated try/except)
2. If MP is not used - it could lead to a problem if someone will read live HDF5 via frozen binaries.

• in this case we should point out that it is not safe to use frozen binaries (Windows/MacOS) to read live data.

the main purpose of 1st solution of @woutdenolf presented here:

1. to isolate the error of reading the live file - so it could be handled explicitly

2nd solution presented by @woutdenolf is to remove MP by using subprocess.Popen:

1. in this case we can remove freeze_support and try/except for MP

• require testing and adjustments

Is subprocess.Popen([sys.executable, "-c", python_code] enough for frozen binaries, no idea.

On the other side if we include MP directly:

1. frozen binaries always use MP to read HDF5
2. no try/except logic is needed
3. Can bring potential issues to freezing procedure and frozen binaries themselves.

• in this case we need to write sufficient tests to verify implementation of MP to be sure it is inserted and works properly.

[sergey-yaroslavtsev]

... it has been giving troubles to generate frozen binaries since day one. Keeping it optional allows to bypass freezing problems by simply excluding the module from the freezing procedure.

Just to clarify:
It was giving a problem only in freezing procedure or it froze successfully and then smthg fails during operation (like this last MacOS bug)? or both?

I know that freezing MP could be painful and have unexpected fails. However, in my previous experience outside of PyMca (which is not a lot to be honest) the MP was either working correctly or not at all. That is why I am asking.

[vasole]

It was giving a problem only in freezing procedure or it froze successfully and then smthg fails during operation (like this last MacOS bug)? or both?

In the frozen binary.