Skip to content

Conversation

cxzhong
Copy link

@cxzhong cxzhong commented Oct 2, 2025

This PR fixes the compatible problem with python 3.14.

  1. Do not call python code in the signal handler. I just use the async signal safe functions. I just add signal_to_raise record the signal and call python exception in the safety context. (fix it with more straight way)
  2. I fixed the compatibility of multiprocess in python 3.14. since in python 3.14, the default start method of multiprocess has changed from fork to forkserver. So, in the multiprocess tests in pselect.pyx, we can not get the signal of SIGCHLD. It will cause the timeout problem in linux. But in macos, the forkserver has different behavior with the behavior in linux. Maybe the wait_for_process is not useful. I just want to monitor the process sentinel file descriptor instead of SIGCHLD.
  3. In python 3.14, since the cpython refactor the way to count the refer numbers, I replaced direct ob_refcnt access with _Py_REFCNT() macro from cpython.ref

cxzhong and others added 3 commits October 1, 2025 16:16
…ork'

Python 3.14 changed the default multiprocessing start method from 'fork'
to 'forkserver' on Linux, which breaks SIGCHLD signal delivery in tests.
This causes pselect tests to hang indefinitely.

Changes:
- Add Python 3.14 detection in conftest.py
- Force multiprocessing to use 'fork' method for Python 3.14+
- Add defensive check for __test__ attribute before deletion

Fixes test timeouts on Python 3.14.0rc2.
All tests now pass in ~8 seconds (previously timed out at 300s+).
…ndlers

Python 3.14 changed recursion checking to use actual C stack pointers
instead of counters. The old implementation called Python code from within
signal handlers before siglongjmp(), causing _Py_CheckRecursiveCall() to
see inconsistent stack pointers and report 'Unrecoverable stack overflow'.

Solution: Never call Python code from within signal handlers.

Changes:
- Added signal_to_raise field to cysigs_t to store deferred signal
- Modified signal handlers to only set flag and longjmp (no Python calls)
- Added _sig_raise_deferred() to raise exceptions after longjmp
- Updated _sig_on_postjmp() to call _sig_raise_deferred() in safe context

This maintains async-signal-safety and ensures Python's stack tracking
remains consistent. All 65 tests pass on Python 3.14.0rc3.

Also updates:
- pyproject.toml: requires-python = '>=3.9,<3.15' (was <3.14)
- CI/CD: Add Python 3.14 to test matrix
@cxzhong
Copy link
Author

cxzhong commented Oct 2, 2025

@tobiasdiez I have tried to fix the signal handle in python 3.14 https://github.com/cxzhong/cysignals/actions/runs/18195804743
And I think there are still problems when building in python 3.14t

cxzhong added 2 commits October 3, 2025 13:26
- Remove forced fork() from conftest.py to allow Python 3.14's default
  forkserver multiprocessing start method
- Add PSelecter.wait_for_process() method that monitors process.sentinel
  instead of SIGCHLD, working with any start method (fork/spawn/forkserver)
- Update doctests to demonstrate both SIGCHLD-based (fork-only) and
  sentinel-based (universal) process monitoring approaches
- Note: wait_for_process() is POSIX-only; Windows should use
  multiprocessing.connection.wait() instead

This allows cysignals to work properly with Python 3.14's new forkserver
default while maintaining backward compatibility.
…refcnt

- Replace direct ob_refcnt access with _Py_REFCNT() macro from cpython.ref
- Add NULL checks before calling _Py_REFCNT() to prevent null pointer dereference
- This fixes compilation errors in Python 3.14t (free-threaded/nogil) build
  where ob_refcnt is not directly accessible in PyObject structure
@cxzhong cxzhong force-pushed the main branch 2 times, most recently from 262520b to 546217f Compare October 3, 2025 05:51
…_REFCNT

- Remove _Py_REFCNT from cpython.ref import (not available in Cython)
- Declare Py_REFCNT as external C function from Python.h
- Replace _Py_REFCNT() calls with Py_REFCNT() (2 occurrences)
- This properly supports Python 3.14t free-threaded build where ob_refcnt
  is not directly accessible in PyObject structure
@cxzhong
Copy link
Author

cxzhong commented Oct 3, 2025

fix #232 and timeout problem in Linux. and I have finally fixed the problems building in python3.14t. The timeout problem is caused by changing of default start method of python 3.14 multiprocess from fork to forkserver.

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

Mostly looks good to me, based on my superficial understanding of the code. @dimpase @ptrrsn @user202729 could you please have a look at this PR as well?

@cxzhong
Copy link
Author

cxzhong commented Oct 4, 2025

@tobiasdiez can you let the workflows run? Thank you.

Remove unused import of sys module.
Replaced usage of Py_REFCNT with _Py_REFCNT for compatibility with Python 3.14t free-threaded builds.
@cxzhong
Copy link
Author

cxzhong commented Oct 4, 2025

Now all tests pass. https://github.com/cxzhong/cysignals/actions/runs/18242832091

@user202729
Copy link
Contributor

instead of adding signal_to_raise, since it's already known by the jmpret value, I suggest just passing that to sig_raise_exception.

The less states there are, the less likely it is to get inconsistent.

Is there any situation where jmpret > 0 yet we don't want to call do_raise_exception?

What does wait_for_process do? It isn't used anywhere.

@user202729
Copy link
Contributor

also, disclose your extent of usage of AI based tools. (most human don't write the pull request description the way you did.)

@cxzhong
Copy link
Author

cxzhong commented Oct 4, 2025

also, disclose your extent of usage of AI based tools. (most human don't write the pull request description the way you did.)

I have rewrited the PR description.

Eliminate redundant signal state by using only sigsetjmp/siglongjmp
return values to propagate signal numbers. This simplifies the signal
handling architecture and prevents potential desynchronization bugs.

Changes:
- Remove signal_to_raise field from cysigs_t structure
- Update signal handlers to not store signal number in global variable
- Remove _sig_raise_deferred() function
- Modify _sig_on_postjmp() to directly use jump return value
- Update Cython declarations accordingly

Benefits:
- Eliminates redundant state (16 lines removed)
- Prevents state desynchronization issues
- Signal number propagates naturally via jump mechanism
- All 66 tests pass successfully

The refactoring maintains full backward compatibility with no
functional changes to the API or behavior.
@cxzhong cxzhong requested a review from tobiasdiez October 4, 2025 17:52
@user202729
Copy link
Contributor

user202729 commented Oct 4, 2025

any idea if this affect anything on WIN32? Given that the siglongjmp is not called on WIN32.

edit: looks like tests pass, so it probably can't be too bad.

@cxzhong
Copy link
Author

cxzhong commented Oct 4, 2025

any idea if this affect anything on WIN32? Given that the siglongjmp is not called on WIN32.

edit: looks like tests pass, so it probably can't be too bad.

Maybe we need to test the compatibility in sagemath. To see everything is OK. Thank you very much.

@cxzhong cxzhong changed the title fix compatible in python 3.14 fix compatible issues in python 3.14 Oct 4, 2025
@dimpase
Copy link
Member

dimpase commented Oct 4, 2025

for SageMath on Python 3.14 we need to fix atexit.pyx first, before we move on to other things

@cxzhong cxzhong requested a review from user202729 October 5, 2025 11:18
@user202729
Copy link
Contributor

user202729 commented Oct 5, 2025

  • what does wait_for_process do, again?
  • in which situation may __test__ be undefined?
  • I said "disclose your AI usage" instead of "rewrite the pull request description". some AI usage are tine, just disclose to which extent.
  • I've no idea what the change to uv.lock do, ask @tobiasdiez .
  • otherwise no serious issue.

@tobiasdiez
Copy link
Contributor

uv updates are fine.

@cxzhong
Copy link
Author

cxzhong commented Oct 5, 2025

  • what does wait_for_process do, again?
  • in which situation may __test__ be undefined?
  • I said "disclose your AI usage" instead of "rewrite the pull request description". some AI usage are tine, just disclose to which extent.
  • I've no idea what the change to uv.lock do, ask @tobiasdiez .
  • otherwise no serious issue.
  1. I will delete this. Soon I will push a commit. It seems useless. I just wrote to verify that I do not use the signal SIGCHLD
  2. I am sorry that I do not know that.
  3. I just used GPT5 to analysis the dangerous behavior and get feedback of an ideal, then I just wrote and let the GitHub copilot verify this. and I also use copilot in VS code to summarize the context of commits.
  4. It is generated by uv automatically.
    Thank you very much for your feedback

@cxzhong
Copy link
Author

cxzhong commented Oct 6, 2025

I have finished tests on nogil python 3.14t in Linux, it can pass all tests if we run pytest.

@cxzhong cxzhong marked this pull request as ready for review October 8, 2025 07:56
@Copilot Copilot AI review requested due to automatic review settings October 8, 2025 07:56
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Align cysignals with Python 3.14 and Cython 3.1, avoiding Python calls in signal handlers, adapting to multiprocessing start-method changes, and updating refcount access for CPython changes.

  • Defer exception raising from signal handlers; use siglongjmp to return and raise in a safe context.
  • Replace direct ob_refcnt with _Py_REFCNT and add Cython free-threading compatibility directives; bump Python/Cython minimums.
  • Update docs/tests to use multiprocessing.get_context and process sentinel; refresh CI runners and Python versions.

Reviewed Changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/cysignals/tests.pyx Add Cython free-threading compatibility directive.
src/cysignals/signals.pyx Use _Py_REFCNT and import; add extern for _do_raise_exception.
src/cysignals/signals.pxd Declare _do_raise_exception (currently under a nogil block).
src/cysignals/pysignals.pyx Add Cython free-threading compatibility directive.
src/cysignals/pselect.pyx Doctest updates to use multiprocessing.get_context and sentinels.
src/cysignals/macros.h Raise exception in safe context via _do_raise_exception in _sig_on_postjmp.
src/cysignals/implementation.c Avoid Python calls in signal handlers; rename do_raise_exception to _do_raise_exception.
src/cysignals/alarm.pyx Add Cython free-threading compatibility directive.
src/conftest.py Safely delete test only if present.
pyproject.toml Bump Cython/Python requirements; update classifiers.
README.rst Update requirements to Python ≥3.11 and Cython ≥3.1.
.github/workflows/dist.yml Update macOS runner to 14.
.github/workflows/ci.yml Add Python 3.14 to matrix; update macOS runner.
Comments suppressed due to low confidence (3)

src/cysignals/implementation.c:1

  • On Windows, the signal handler no longer raises an exception or returns via a trampoline; the previous do_raise_exception(sig) call was removed and there's no alternative in the _WIN32 path. Add an #else branch that calls _do_raise_exception(sig) on Windows to avoid losing interrupts.
/*

src/cysignals/implementation.c:1

  • Same issue for non-SIGINT signals: on Windows, the handler now does neither a longjmp nor raises an exception. Add an _WIN32 branch that calls _do_raise_exception(sig) to preserve behavior on Windows.
/*

src/cysignals/signals.pxd:1

  • _do_raise_exception raises Python exceptions and must run with the GIL, but it's declared inside a cdef nogil block, permitting calls without the GIL. Move this declaration out of the nogil block (declare it under a normal cdef:) or ensure _do_raise_exception acquires/releases the GIL internally in all code paths.
# cython: preliminary_late_includes_cy28=True

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

static void cysigs_signal_handler(int sig);

static void do_raise_exception(int sig);
static void _do_raise_exception(int sig);
Copy link
Preview

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_do_raise_exception is declared static here, but it is invoked from macros.h's inline _sig_on_postjmp, which is compiled into other translation units. With static linkage, other TUs cannot resolve this symbol, causing link errors. Remove static (make it have external linkage) and provide a prototype in a shared header (e.g., macros.h) so all TUs see the declaration.

Suggested change
static void _do_raise_exception(int sig);
void _do_raise_exception(int sig);

Copilot uses AI. Check for mistakes.

Comment on lines +157 to +161
/* A signal occurred and we jumped back via longjmp.
* jmpret contains the signal number that was passed to siglongjmp.
* Now we're back in a safe context (not in signal handler),
* so it's safe to call Python code to raise the exception. */
_do_raise_exception(jmpret);
Copy link
Preview

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This inline function calls _do_raise_exception but there's no C declaration visible in this header. Without an extern prototype, this will be an implicit declaration (or an error under modern compilers). Add an extern prototype for _do_raise_exception(int) in a header included by all consumers (e.g., declare it at the top of macros.h) and ensure the function has external linkage.

Copilot uses AI. Check for mistakes.

* Return 0 if there was an exception, 1 otherwise.
*
* This function propagates the signal number naturally via jmpret
* (the return value from sigsetjmp/cylongjmp), which is always nonzero
Copy link
Preview

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct 'cylongjmp' to 'siglongjmp'.

Suggested change
* (the return value from sigsetjmp/cylongjmp), which is always nonzero
* (the return value from sigsetjmp/siglongjmp), which is always nonzero

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants