diff --git a/include/methods.h b/include/methods.h index 605c1279..01cf495d 100644 --- a/include/methods.h +++ b/include/methods.h @@ -436,7 +436,10 @@ extern "C" MODULE_NAME \ ".set_reflash_callback(callback)\n" \ "\n" \ - "Sets the reflash display callback.\n" \ + "Sets the reflash display callback. The callback is retained until replaced or disabled.\n" \ + "An object with a callable reflash_callback method is also accepted.\n" \ + "Pass None to disable callbacks, or omit the argument to print progress to stdout.\n" \ + "Callback exceptions are reported through sys.unraisablehook.\n" \ "\n" \ "Args:\n" \ "\tcallback (:class:`function`): Must be a callable Python function (`def callback(msg, progress)`)\n\n" \ diff --git a/src/methods.cpp b/src/methods.cpp index 1fde87fd..01dfb14c 100644 --- a/src/methods.cpp +++ b/src/methods.cpp @@ -2139,18 +2139,45 @@ PyObject* meth_flash_devices(PyObject* self, PyObject* args) } #endif // _USE_INTERNAL_HEADER_ -PyObject* msg_reflash_callback = NULL; +// Owned reference, accessed with the GIL held. NULL selects stdout output. +static PyObject* msg_reflash_callback = NULL; static void message_reflash_callback(const wchar_t* message, unsigned long progress) { // We need to relock the GIL here otherwise we crash PyGILState_STATE state = PyGILState_Ensure(); - if (!msg_reflash_callback) { - PySys_WriteStdout("%ls -%ld\n", message, progress); - } else if (PyObject_HasAttrString(msg_reflash_callback, "reflash_callback")) { - PyObject_CallMethod(msg_reflash_callback, "reflash_callback", "u,k", message, progress); + // Keep the current handler alive even if it unregisters/replaces itself. + PyObject* callback = msg_reflash_callback; + Py_XINCREF(callback); + if (!callback) { + PyObject* text = PyUnicode_FromWideChar(message, -1); + if (text) { + PySys_FormatStdout("%U -%lu\n", text, progress); + Py_DECREF(text); + } else { + PyErr_WriteUnraisable(Py_None); + } } else { - PyObject_CallFunction(msg_reflash_callback, "u,k", message, progress); + PyObject* callable = PyObject_GetAttrString(callback, "reflash_callback"); + if (!callable && PyErr_ExceptionMatches(PyExc_AttributeError)) { + PyErr_Clear(); + callable = callback; + Py_INCREF(callable); + } + if (callable) { + PyObject* text = PyUnicode_FromWideChar(message, -1); + if (text) { + PyObject* result = PyObject_CallFunction(callable, "Ok", text, progress); + Py_XDECREF(result); + Py_DECREF(text); + } + Py_DECREF(callable); + } + // Native progress notifications have no Python caller to receive errors. + if (PyErr_Occurred()) { + PyErr_WriteUnraisable(callback); + } } + Py_XDECREF(callback); // Unlock the GIL here again... PyGILState_Release(state); } @@ -2163,10 +2190,22 @@ PyObject* meth_set_reflash_callback(PyObject* self, PyObject* args) if (!PyArg_ParseTuple(args, arg_parse("|O:", __FUNCTION__), &callback)) { return NULL; } - if (!callback) { - msg_reflash_callback = NULL; - } else { - msg_reflash_callback = callback; + if (callback && callback != Py_None) { + PyObject* callable = PyObject_GetAttrString(callback, "reflash_callback"); + if (!callable) { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { + return NULL; + } + PyErr_Clear(); + callable = callback; + Py_INCREF(callable); + } + int is_callable = PyCallable_Check(callable); + Py_DECREF(callable); + if (!is_callable) { + PyErr_SetString(PyExc_TypeError, "callback must be callable or have a callable reflash_callback method"); + return NULL; + } } try { ice::Library* lib = dll_get_library(); @@ -2176,6 +2215,12 @@ PyObject* meth_set_reflash_callback(PyObject* self, PyObject* args) } ice::Function icsneoSetReflashCallback( lib, "icsneoSetReflashCallback"); + // Resolve the library symbol before changing ownership. Publish before + // calling native code, which may immediately deliver a progress event. + PyObject* replacement = callback == Py_None ? NULL : callback; + Py_XINCREF(replacement); + PyObject* previous = msg_reflash_callback; + msg_reflash_callback = replacement; auto gil = PyAllowThreads(); if (callback == Py_None) { icsneoSetReflashCallback(NULL); @@ -2183,6 +2228,7 @@ PyObject* meth_set_reflash_callback(PyObject* self, PyObject* args) icsneoSetReflashCallback(&message_reflash_callback); } gil.restore(); + Py_XDECREF(previous); Py_RETURN_NONE; } catch (ice::Exception& ex) { return set_ics_exception(exception_runtime_error(), (char*)ex.what()); diff --git a/tests/test_reflash_callback.py b/tests/test_reflash_callback.py new file mode 100644 index 00000000..c828fc17 --- /dev/null +++ b/tests/test_reflash_callback.py @@ -0,0 +1,302 @@ +"""Hardware-free callback tests; each case owns an isolated native-library registration.""" + +import os +from pathlib import Path +import shutil +import subprocess +import sys + +import pytest + + +@pytest.fixture(scope="module") +def callback_libraries(tmp_path_factory): + directory = tmp_path_factory.mktemp("reflash_callback") + source = directory / "callback.c" + source.write_text(r""" +#ifdef _WIN32 +#define API __declspec(dllexport) +#define CALL __stdcall +typedef unsigned short wchar_t; +#else +#include +#define API __attribute__((visibility("default"))) +#define CALL +#endif +typedef void (*callback_t)(const wchar_t*, unsigned long); +static callback_t callback; +static int fire_on_registration; +API void fire_callback(void); +API void set_fire_on_registration(int value) { fire_on_registration = value; } +#ifndef OMIT_SETTER +API void CALL icsneoSetReflashCallback(callback_t value) { + callback = value; + if (fire_on_registration) fire_callback(); +} +#endif +API void fire_callback(void) { + static const wchar_t message[] = { 'p', 'r', 'o', 'g', 'r', 'e', 's', 's', 0x2713, 0 }; + if (callback) callback(message, 42); +} +""") + libraries = [] + for missing in (False, True): + name = "missing" if missing else "callback" + if sys.platform == "win32": + compiler = shutil.which("clang-cl") + linker = shutil.which("lld-link") + if not compiler or not linker: + pytest.skip("callback mock requires clang-cl and lld-link") + library = directory / (name + ".dll") + obj = directory / (name + ".obj") + target = "i686-pc-windows-msvc" if sys.maxsize <= 2**32 else "x86_64-pc-windows-msvc" + command = [compiler, "--target=" + target, "/nologo", "/c", "/GS-", "/Zl", str(source), "/Fo" + str(obj)] + if missing: + command.append("/DOMIT_SETTER") + subprocess.run(command, check=True, capture_output=True) + exports = ( + ["/export:fire_callback=_fire_callback", "/export:set_fire_on_registration=_set_fire_on_registration"] + if sys.maxsize <= 2**32 + else [] + ) + if sys.maxsize <= 2**32 and not missing: + exports.append("/export:icsneoSetReflashCallback=_icsneoSetReflashCallback@4") + subprocess.run( + [linker, "/dll", "/noentry", "/nodefaultlib", "/out:" + str(library), str(obj), *exports], + check=True, + capture_output=True, + ) + else: + compiler = shutil.which("cc") + if not compiler: + pytest.skip("callback mock requires a C compiler") + library = directory / (name + (".dylib" if sys.platform == "darwin" else ".so")) + command = [ + compiler, + "-dynamiclib" if sys.platform == "darwin" else "-shared", + "-fPIC", + str(source), + "-o", + str(library), + ] + if missing: + command.append("-DOMIT_SETTER") + subprocess.run(command, check=True, capture_output=True) + libraries.append(library) + return libraries + + +PREAMBLE = """ +import ctypes, gc, sys, weakref +import ics +ics.override_library_name(sys.argv[1]) +native = ctypes.CDLL(sys.argv[1]) +native.fire_callback.argtypes = [] +native.fire_callback.restype = None +events = [] +class Handler: + def __call__(self, message, progress): + events.append((message, progress)) +def collect(): + gc.collect() +""" + + +CASES = { + "lifetime": """ +handler = Handler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +collect() +assert ref() is not None, 'registered callback was collected' +native.fire_callback() +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +collect() +assert ref() is None +native.fire_callback() +assert len(events) == 1 +""", + "replacement": """ +first, second = Handler(), Handler() +old, new = weakref.ref(first), weakref.ref(second) +ics.set_reflash_callback(first) +ics.set_reflash_callback(first) +del first +ics.set_reflash_callback(second) +del second +collect() +assert old() is None and new() is not None +ics.set_reflash_callback(None) +collect() +assert new() is None +""", + "method": """ +class MethodHandler: + def reflash_callback(self, message, progress): + events.append((message, progress)) +handler = MethodHandler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +collect() +assert ref() is not None +native.fire_callback() +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +collect() +assert ref() is None +""", + "invalid": """ +handler = Handler() +ics.set_reflash_callback(handler) +class Invalid: + reflash_callback = 1 +for value in (123, object(), Invalid()): + try: + ics.set_reflash_callback(value) + except TypeError: + pass + else: + raise AssertionError('accepted invalid callback') +native.fire_callback() +assert len(events) == 1 +ics.set_reflash_callback(None) +""", + "return_value": """ +refs = [] +def callback(message, progress): + result = Handler() + refs.append(weakref.ref(result)) + return result +ics.set_reflash_callback(callback) +for _ in range(10): + native.fire_callback() +collect() +assert all(ref() is None for ref in refs) +ics.set_reflash_callback(None) +""", + "exception": """ +errors = [] +sys.unraisablehook = lambda error: errors.append((error.exc_type, str(error.exc_value))) +def callback(message, progress): + raise ValueError('callback failed') +ics.set_reflash_callback(callback) +native.fire_callback() +native.fire_callback() +assert errors == [(ValueError, 'callback failed')] * 2 +ics.set_reflash_callback(Handler()) +native.fire_callback() +assert len(events) == 1 +ics.set_reflash_callback(None) +""", + "reentrant": """ +refs = [] +errors = [] +sys.unraisablehook = lambda error: errors.append(error.exc_type) +class Reentrant: + def __call__(self, message, progress): + ics.set_reflash_callback(None) + collect() + assert refs[0]() is self + raise ValueError('after unregister') +handler = Reentrant() +refs.append(weakref.ref(handler)) +ics.set_reflash_callback(handler) +del handler +native.fire_callback() +collect() +assert errors == [ValueError] +assert refs[0]() is None +""", + "stdout": """ +import contextlib, io +handler = Handler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +ics.set_reflash_callback() +collect() +assert ref() is None +output = io.StringIO() +with contextlib.redirect_stdout(output): + native.fire_callback() +assert output.getvalue() == 'progress\\u2713 -42\\n' +ics.set_reflash_callback(None) +""", + "missing_symbol": """ +handler = Handler() +ref = weakref.ref(handler) +ics.set_reflash_callback(handler) +del handler +ics.override_library_name(sys.argv[2]) +candidate = Handler() +candidate_ref = weakref.ref(candidate) +try: + ics.set_reflash_callback(candidate) +except ics.RuntimeError: + pass +else: + raise AssertionError('missing symbol accepted') +del candidate +collect() +assert ref() is not None and candidate_ref() is None +native.fire_callback() +assert len(events) == 1 +ics.override_library_name(sys.argv[1]) +ics.set_reflash_callback(None) +collect() +assert ref() is None +""", + "attribute_error": """ +class Broken: + @property + def reflash_callback(self): + raise ValueError('attribute failed') +ics.set_reflash_callback(Handler()) +try: + ics.set_reflash_callback(Broken()) +except ValueError as error: + assert str(error) == 'attribute failed' +else: + raise AssertionError('attribute error suppressed') +native.fire_callback() +assert len(events) == 1 +ics.set_reflash_callback(None) +""", + "thread": """ +import threading +ics.set_reflash_callback(Handler()) +worker = threading.Thread(target=native.fire_callback) +worker.start() +worker.join(timeout=5) +assert not worker.is_alive() +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +""", + "immediate": """ +native.set_fire_on_registration.argtypes = [ctypes.c_int] +native.set_fire_on_registration.restype = None +native.set_fire_on_registration(1) +ics.set_reflash_callback(Handler()) +assert events == [('progress\\u2713', 42)] +ics.set_reflash_callback(None) +assert len(events) == 1 +""", +} + + +@pytest.mark.parametrize("case", CASES) +def test_reflash_callback(callback_libraries, case): + env = os.environ.copy() + # Preserve the selected checkout/build when launching an isolated interpreter. + env["PYTHONPATH"] = os.pathsep.join(str(Path(path).resolve()) for path in sys.path) + result = subprocess.run( + [sys.executable, "-c", PREAMBLE + CASES[case], *map(str, callback_libraries)], + env=env, + capture_output=True, + text=True, + timeout=30, + ) + assert result.returncode == 0, result.stdout + result.stderr