fix: ensure we do not segfault when using threads and nogil (#486)

beckermr · web-flow · commit 8c5d2148fdb6 · 2026-02-06T08:08:40.000-06:00
* fix: ensure we do not segfault when using threads with nogil

* style: pre-commit

* doc: clarify changelog

* prod: run testing on slim images

* fix: this does notwork
diff --git a/.github/workflows/tests-external-cfitsio.yml b/.github/workflows/tests-external-cfitsio.yml
@@ -30,17 +30,19 @@ jobs:
         os: [macos-latest, ubuntu-latest]
         config:
           # 3.44 is the last version that did not support uint64
-          - { pyver: "3.9", npver: "1", cftsver: "3440", doslow: ""}
+          - { pyver: "3.9", npver: "1", cftsver: "3440", doslow: "", ft: "", cf: ""}
           # python 3 string writing fails on all 3.* builds
           # so test first 4.* release
-          - { pyver: "3.9", npver: "1", cftsver: "-4.0.0", doslow: ""}
+          - { pyver: "3.9", npver: "1", cftsver: "-4.0.0", doslow: "", ft: "", cf: ""}
           # 4.1.0 is the first version for which tests pass for uint64
-          - { pyver: "3.9", npver: "1", cftsver: "-4.1.0", doslow: ""}
+          - { pyver: "3.9", npver: "1", cftsver: "-4.1.0", doslow: "", ft: "", cf: ""}
           # 4.4.0 is first version for which tests pass for compressed
           # binary tables that exceed 2**32 bytes
-          - { pyver: "3.9", npver: "1", cftsver: "-4.4.0", doslow: "--slow"}
-          - { pyver: "3.9", npver: "1", cftsver: "latest", doslow: "--slow"}
-          - { pyver: "3.14", npver: "2", cftsver: "latest", doslow: "--slow"}
+          - { pyver: "3.9", npver: "1", cftsver: "-4.4.0", doslow: "--slow", ft: "", cf: ""}
+          - { pyver: "3.9", npver: "1", cftsver: "latest", doslow: "--slow", ft: "", cf: ""}
+          - { pyver: "3.14", npver: "2", cftsver: "latest", doslow: "--slow", ft: "python-gil", cf: "--enable-reentrant"}
+          - { pyver: "3.14", npver: "2", cftsver: "latest", doslow: "--slow", ft: "python-freethreading", cf: ""}
+          - { pyver: "3.14", npver: "2", cftsver: "latest", doslow: "--slow", ft: "python-freethreading", cf: "--enable-reentrant"}
 
     runs-on: ${{ matrix.os }}
     env:
@@ -74,33 +76,46 @@ jobs:
           create-args: >-
             python=${{ matrix.config.pyver }}
             numpy=${{ matrix.config.npver }}
+            ${{ matrix.config.ft }}
 
-      - name: build external cfitsio
+      - name: set parallel testing flags
         run: |
-          export CFLAGS="${CFLAGS} ${TEST_CFLAGS}"
-
-          mkdir -p cfitsio-external-build
-          cd cfitsio-external-build
-          rm -rf *
+          if [[ "${{ matrix.config.ft }}" == "python-freethreading" ]]; then
+            echo "PRP_FLAGS=--parallel-threads 4 --iterations 4 --ignore-gil-enabled" >> ${GITHUB_ENV}
+          fi
 
+      - name: set cfitsio config flags and version
+        run: |
           if [[ "${{ matrix.config.cftsver }}" == "latest" ]]; then
             cftsver=${LATEST_CFITSIO_VER}
           else
             cftsver="${{ matrix.config.cftsver }}"
           fi
 
+          echo "CFITSIO_VER=${cftsver}" >> ${GITHUB_ENV}
+
           if [[ "${{ matrix.config.cftsver }}" == *3* ]]; then
-            config_flags=""
+            config_flags="${{ matrix.config.cf }}"
           else
-            config_flags="--without-fortran --disable-shared"
+            config_flags="--without-fortran --disable-shared ${{ matrix.config.cf }}"
           fi
 
-          cfitsio_name=cfitsio${cftsver}
+          echo "CFITSIO_CONFIG_FLAGS=${config_flags}" >> ${GITHUB_ENV}
+
+      - name: build external cfitsio
+        run: |
+          export CFLAGS="${CFLAGS} ${TEST_CFLAGS} -fPIC"
+
+          mkdir -p cfitsio-external-build
+          cd cfitsio-external-build
+          rm -rf *
+
+          cfitsio_name=cfitsio${CFITSIO_VER}
           wget https://heasarc.gsfc.nasa.gov/FTP/software/fitsio/c/${cfitsio_name}.tar.gz
           cfitsio_dir=`tar -tzf ${cfitsio_name}.tar.gz | sed -n "1,1p" | cut -f1 -d"/"`
           tar -xzvf ${cfitsio_name}.tar.gz
           cd ${cfitsio_dir}
-          CFLAGS="-fPIC" ./configure --prefix=$HOME/cfitsio-static-install ${config_flags}
+          ./configure --prefix=$HOME/cfitsio-static-install ${CFITSIO_CONFIG_FLAGS}
           make install -j 4
           cd ..
           cd ..
@@ -121,7 +136,7 @@ jobs:
             python -c "import fitsio; assert fitsio.cfitsio_has_curl_support()"
           fi
 
-          pytest -vv fitsio
+          pytest -vv ${PRP_FLAGS} fitsio
 
       - name: install bzip2 on linux
         if: matrix.os == 'ubuntu-latest'
@@ -130,30 +145,18 @@ jobs:
 
       - name: build external cfitsio w/ bzip2
         run: |
-          export CFLAGS="${CFLAGS} ${TEST_CFLAGS}"
+          export CFLAGS="${CFLAGS} ${TEST_CFLAGS} -fPIC"
 
           mkdir -p cfitsio-external-build
           cd cfitsio-external-build
           rm -rf *
 
-          if [[ "${{ matrix.config.cftsver }}" == "latest" ]]; then
-            cftsver=${LATEST_CFITSIO_VER}
-          else
-            cftsver="${{ matrix.config.cftsver }}"
-          fi
-
-          if [[ "${{ matrix.config.cftsver }}" == *3440* ]]; then
-            config_flags="--with-bzip2"
-          else
-            config_flags="--without-fortran --disable-shared --with-bzip2"
-          fi
-
-          cfitsio_name=cfitsio${cftsver}
+          cfitsio_name=cfitsio${CFITSIO_VER}
           wget https://heasarc.gsfc.nasa.gov/FTP/software/fitsio/c/${cfitsio_name}.tar.gz
           cfitsio_dir=`tar -tzf ${cfitsio_name}.tar.gz | sed -n "1,1p" | cut -f1 -d"/"`
           tar -xzvf ${cfitsio_name}.tar.gz
           cd ${cfitsio_dir}
-          CFLAGS="-fPIC" ./configure --prefix=$HOME/cfitsio-static-install ${config_flags}
+          ./configure --prefix=$HOME/cfitsio-static-install ${CFITSIO_CONFIG_FLAGS} --with-bzip2
           make install -j 4
           cd ..
           cd ..
@@ -177,4 +180,4 @@ jobs:
             python -c "import fitsio; assert fitsio.cfitsio_has_curl_support()"
           fi
 
-          pytest -vv ${{ matrix.config.doslow }} fitsio
+          pytest -vv ${{ matrix.config.doslow }} ${PRP_FLAGS} fitsio
diff --git a/.github/workflows/tests-pypi.yml b/.github/workflows/tests-pypi.yml
@@ -22,7 +22,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        os: [macos-latest, ubuntu-22.04]
+        os: [macos-latest, ubuntu-latest]
         pyver: ["3.9", "3.14"]
     runs-on: ${{ matrix.os }}
 
diff --git a/CHANGES.md b/CHANGES.md
@@ -15,7 +15,7 @@ Changes
     - Marked some tests as slow to speed up testing. Pass `--slow` to
       pytest to run them.
     - Added python 3.14 to the CI config.
-    - Added testing against free threaded builds of python.
+    - Added support and testing against free threaded builds of python.
 
 Bug Fixes
 
diff --git a/fitsio/fitsio_pywrap.c b/fitsio/fitsio_pywrap.c
@@ -5433,7 +5433,9 @@ init_fitsio_wrap(void)
 
 #if PY_MAJOR_VERSION >= 3
 #ifdef Py_GIL_DISABLED
-    PyUnstable_Module_SetGIL(m, Py_MOD_GIL_NOT_USED);
+    if (fits_is_reentrant() != 0) {
+        PyUnstable_Module_SetGIL(m, Py_MOD_GIL_NOT_USED);
+    }
 #endif
 #endif
 
diff --git a/fitsio/tests/test_threading.py b/fitsio/tests/test_threading.py
@@ -0,0 +1,211 @@
+from concurrent.futures import ThreadPoolExecutor, as_completed
+import os
+import tempfile
+import time
+
+import numpy as np
+import fitsio
+
+import pytest
+
+SIZE = 5000
+DATA = np.zeros((SIZE, SIZE), dtype='f8')
+DATA[:] = -1
+
+
+def create_file(fname):
+    with fitsio.FITS(fname, 'rw') as fits:
+        fits.write_image(DATA)
+        fits[0].write_checksum()
+        fits.read_raw()
+
+
+def read_file(fname):
+    with fitsio.FITS(fname, 'r') as fits:
+        fits[0].verify_checksum()
+        fits[0].read()
+
+
+def test_threading_works():
+    """
+    Test a basic image write, data and a header, then reading back in to
+    check the values
+    """
+    nt = 32
+
+    with tempfile.TemporaryDirectory() as tmpdir:
+        filenames = [
+            os.path.join(tmpdir, "fname%d.fits" % i) for i in range(nt)
+        ]
+
+        def _create_file(i):
+            fname = filenames[i]
+            data = np.zeros((32, 32), dtype='f8')
+            data[:] = i
+            with fitsio.FITS(fname, 'rw', clobber=True) as fits:
+                fits.write_image(data)
+                fits[0].write_checksum()
+
+        def _read_file(i):
+            fname = filenames[i]
+            with fitsio.FITS(fname, 'r') as fits:
+                fits[0].verify_checksum()
+                assert (fits[0].read() == i).all()
+
+        with ThreadPoolExecutor(max_workers=nt) as pool:
+            for _ in pool.map(_create_file, range(nt)):
+                pass
+            for _ in pool.map(_read_file, range(nt)):
+                pass
+
+
+@pytest.mark.slow
+@pytest.mark.xfail(reason="Threading performance might be flaky!")
+@pytest.mark.parametrize(
+    "write_only,read_only",
+    [
+        (False, True),
+        (True, False),
+        (False, False),
+    ],
+)
+@pytest.mark.parametrize("klass", [ThreadPoolExecutor])
+def test_threading_timing(klass, write_only, read_only):
+    """
+    Test a basic image write, data and a header, then reading back in to
+    check the values
+    """
+    nt = 2
+    fac = 2
+
+    if read_only:
+        print("\nread only", flush=True)
+    elif write_only:
+        print("\nwrite only", flush=True)
+    else:
+        print("\nread and write", flush=True)
+
+    with tempfile.TemporaryDirectory() as tmpdir:
+        filenames = [
+            os.path.join(tmpdir, "fname%d.fits" % i) for i in range(nt * fac)
+        ]
+
+        def _remove_files():
+            for fname in filenames:
+                try:
+                    os.remove(fname)
+                except Exception:
+                    pass
+
+        if read_only:
+            for fname in filenames:
+                create_file(fname)
+
+        t0 = time.time()
+        if not read_only:
+            create_file(filenames[0])
+        if not write_only:
+            read_file(filenames[0])
+        t0_one = time.time() - t0
+        print("one file time:", t0_one, flush=True)
+        if not read_only:
+            _remove_files()
+
+        t0 = time.time()
+        with klass(max_workers=nt) as pool:
+            if not read_only:
+                futs = [
+                    pool.submit(create_file, filenames[i])
+                    for i in range(nt * fac)
+                ]
+                for fut in as_completed(futs):
+                    fut.result()
+            if not write_only:
+                futs = [
+                    pool.submit(read_file, filenames[i])
+                    for i in range(nt * fac)
+                ]
+                for fut in as_completed(futs):
+                    fut.result()
+        t0_threads = time.time() - t0
+        print(
+            "parallel time / one file time",
+            t0_threads / t0_one,
+            "(perfect is %d)" % fac,
+            flush=True,
+        )
+        if not read_only:
+            _remove_files()
+
+        t0 = time.time()
+        if not read_only:
+            for fname in filenames:
+                create_file(fname)
+        if not write_only:
+            for fname in filenames:
+                read_file(fname)
+        t0_serial = time.time() - t0
+        print(
+            "serial time / one file time:",
+            t0_serial / t0_one,
+            f"(should be about {nt * fac})",
+            flush=True,
+        )
+
+        if read_only:
+            assert t0_threads < t0_serial, (
+                "Threading should be faster than serial! ( %f < %f)"
+                % (t0_threads, t0_serial)
+            )
+
+
+@pytest.mark.slow
+@pytest.mark.xfail(reason="Threading performance might be flaky!")
+def test_threading_read_one_file():
+    nt = 4
+
+    with tempfile.TemporaryDirectory() as tmpdir:
+        fname = os.path.join(tmpdir, "fname.fits")
+        with fitsio.FITS(fname, 'rw', clobber=True) as fits:
+            fits.write_image(np.concatenate([DATA]), compress="GZIP", qlevel=0)
+            fits[0].write_checksum()
+
+        def _read_file(fname):
+            with fitsio.FITS(fname, 'r') as fits:
+                fits[0].verify_checksum()
+                assert (fits[1].read() == -1).all()
+            return True
+
+        t0 = time.time()
+        _read_file(fname)
+        t0_one = time.time() - t0
+        print("\none file time:", t0_one, flush=True)
+
+        t0 = time.time()
+        with ThreadPoolExecutor(max_workers=nt) as pool:
+            futs = [pool.submit(_read_file, fname) for _ in range(nt)]
+
+            assert all([fut.result() for fut in as_completed(futs)])
+        t0_threads = time.time() - t0
+        print(
+            "parallel time / one file time",
+            t0_threads / t0_one,
+            "(perfect is 1)",
+            flush=True,
+        )
+
+        t0 = time.time()
+        for _ in range(nt):
+            _read_file(fname)
+        t0_serial = time.time() - t0
+        print(
+            "serial time / one file time:",
+            t0_serial / t0_one,
+            f"(should be about {nt})",
+            flush=True,
+        )
+
+        assert t0_threads < t0_serial, (
+            "Threading should be faster than serial! ( %f < %f)"
+            % (t0_threads, t0_serial)
+        )
