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 sagemath/sage#36944

[milahu]

the diff looks wild because of the indent-change

diff minus indent

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