Add option `use_cysignals`

[mkoeppe]

SageMath uses https://github.com/sagemath/cysignals for advanced interrupt and signal handling with Cython modules.

We add an option JobClient(use_cysignals=True) (default: False). With the option set, it imports a context manager changesignal from cysignals, which replaces the use of the standard library function signals.signal.

This new option helps us solve https://github.com/sagemath/sage/issues/36944

[milahu]

the diff looks wild because of the indent-change

diff minus indent

```diff diff --git a/py/src/gnumake_tokenpool/tokenpool.py b/py/src/gnumake_tokenpool/tokenpool.py index b0708ea..f67eb54 100644 --- a/py/src/gnumake_tokenpool/tokenpool.py +++ b/py/src/gnumake_tokenpool/tokenpool.py @@ -1,8 +1,9 @@ import sys, os, stat, select, signal, time, re + +from contextlib import contextmanager from datetime import datetime from typing import List - __version__ = '0.0.3' @@ -26,6 +27,7 @@ class JobClient: max_load: int or None = None, debug: bool or None = None, debug2: bool or None = None, + use_cysignals: bool = False, ): self._fdRead = None @@ -47,6 +49,10 @@ class JobClient: self._log = self._get_log(self._debug) self._log2 = self._get_log(self._debug2) + if use_cysignals: + from cysignals import changesignal + self._changesignal = changesignal + makeFlags = os.environ.get("MAKEFLAGS", "") if makeFlags: self._log(f"init: MAKEFLAGS: {makeFlags}") @@ -181,8 +187,8 @@ class JobClient: os.close(self._fdReadDup) # SIGALRM = timer has fired = read timeout - old_sigalrm_handler = signal.signal(signal.SIGALRM, read_timeout_handler) - + with self._changesignal(signal.SIGALRM, read_timeout_handler): + try: # Set SA_RESTART to limit EINTR occurrences. # by default, signal.signal clears the SA_RESTART flag. # TODO is this necessary? @@ -206,12 +212,9 @@ class JobClient: self._log(f"acquire: read failed: {e}") return None # jobserver is full, try again later raise e # unexpected error - + finally: signal.setitimer(signal.ITIMER_REAL, 0) # clear timer. unix only - # clear signal handlers - signal.signal(signal.SIGALRM, old_sigalrm_handler) - #if len(buffer) == 0: # return None assert len(buffer) == 1 @@ -292,3 +295,13 @@ class JobClient: self._log(f"init failed: fd {fd} stat failed: {e}") raise NoJobServer() raise e # unexpected error + + @staticmethod + @contextmanager + def _changesignal(sig, action): + old_sig_handler = signal.signal(sig, action) + try: + yield + finally: + # clear signal handler + signal.signal(sig, old_sig_handler) ```

looks good : )

if you have too much time (hah...) you could split it into 2 commits: py: use signal context and py: add option use_cysignals

otherwise, feel free to merge

[milahu]

would it make sense to automatically enable cysignals when available?

something like

try:
  from cysignals import changesignal
  self._log("using cysignals.changesignal")
  self._changesignal = changesignal
except ModuleNotFoundError:
  pass

i guess that cysignals has no significant downsides compared to signals

[mkoeppe]

would it make sense to automatically enable cysignals when available?

I can make that a third, default option.

[mkoeppe]

you could split it into 2 commits:

done

[mkoeppe]

I can make that a third, default option.

done in 32e735b

[mkoeppe]

I'll merge it and cut a new release

[mkoeppe]

https://pypi.org/project/gnumake-tokenpool/0.0.4/

[milahu]

thanks : )

for the record: the messing with signals is needed to get a "read with timeout"

ideally, python's f.read would allow

with open("/dev/stdin") as f:
    text = f.read(timeout=5)

but that fails with

TypeError: TextIOWrapper.read() takes no keyword arguments

also python's input function has no timeout parameter

in javascript im using child_process.spawnSync to "read with timeout" (calling dd which only works on linux...)

  const reader = child_process.spawnSync('dd', ddArgs, {
    timeout,
    stdio,
    windowsHide: true,
    ...options,
  });

so in python we could use subprocess.run

#!/usr/bin/env python3

import sys
import subprocess

fd_read = 0 # stdin # TODO use the jobserver's fd_read

# pipe one byte from fd_read to stdout
py_code = f"""
import sys; sys.stdout.buffer.write(open({fd_read}, "rb").read(1))
"""

args = [
  sys.executable, # python
  "-c", py_code,
]

try:
  input_byte = subprocess.run(
    args,
    capture_output=True,
    timeout=2,
  ).stdout
except subprocess.TimeoutExpired:
  input_byte = None

print("input_byte", input_byte)

$ ./test-read.py
input_byte None

$ echo xy | ./test-read.py
input_byte b'x'

[mkoeppe]

Thanks for the explanation! For now, I think, the solution with signals works well though