From 9bf16acf89060d15b1749ea30c76af4623d82862 Mon Sep 17 00:00:00 2001 From: Benoit Chesneau Date: Sat, 29 Aug 2026 18:03:24 +0200 Subject: [PATCH] Add the decision records Eight records, one per design decision that shapes the code today, each with the situation, what was decided, what it costs and where the code is, so a change that reverses one is made knowingly. --- docs/architecture.md | 3 +- docs/contributing.md | 2 + docs/decisions/0001-one-thread-per-context.md | 36 ++++++++++++++++ .../decisions/0002-callback-delivery-paths.md | 43 +++++++++++++++++++ .../decisions/0003-callback-results-as-etf.md | 32 ++++++++++++++ .../0004-isolated-mode-child-process.md | 40 +++++++++++++++++ docs/decisions/0005-py-isolated-gen-statem.md | 39 +++++++++++++++++ .../0006-shared-memory-over-iommap.md | 42 ++++++++++++++++++ .../0007-remove-legacy-execution-paths.md | 31 +++++++++++++ docs/decisions/0008-pipe-io-rules.md | 31 +++++++++++++ docs/decisions/overview.md | 18 ++++++++ rebar.config | 20 +++++++++ 12 files changed, 336 insertions(+), 1 deletion(-) create mode 100644 docs/decisions/0001-one-thread-per-context.md create mode 100644 docs/decisions/0002-callback-delivery-paths.md create mode 100644 docs/decisions/0003-callback-results-as-etf.md create mode 100644 docs/decisions/0004-isolated-mode-child-process.md create mode 100644 docs/decisions/0005-py-isolated-gen-statem.md create mode 100644 docs/decisions/0006-shared-memory-over-iommap.md create mode 100644 docs/decisions/0007-remove-legacy-execution-paths.md create mode 100644 docs/decisions/0008-pipe-io-rules.md create mode 100644 docs/decisions/overview.md diff --git a/docs/architecture.md b/docs/architecture.md index 37588ae..7c30bf3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -129,7 +129,8 @@ same context are dispatched immediately because they come from that process. Every message and frame, with the rules for changing them, is in [protocols](protocols.md); the states each process and thread moves through -are in [state machines](state-machines.md). +are in [state machines](state-machines.md); the reasons behind the design +are in the [decision records](decisions/overview.md). ## asyncio diff --git a/docs/contributing.md b/docs/contributing.md index 825c7e7..52c1226 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -205,6 +205,8 @@ supervisor to carry it. clean locally. - `CHANGELOG.md` updated under the unreleased version: `Added`, `Changed`, `Removed` or `Fixed`. Removing a public function is a major version. +- A change that reverses or extends a decision in `docs/decisions/` gets a + new record there; the old one is not edited. - The PR text says what the change intends and which path it takes; the diff already lists the files. - One squashed commit per PR. diff --git a/docs/decisions/0001-one-thread-per-context.md b/docs/decisions/0001-one-thread-per-context.md new file mode 100644 index 0000000..33fe2bc --- /dev/null +++ b/docs/decisions/0001-one-thread-per-context.md @@ -0,0 +1,36 @@ +# 0001: One pthread per context, NIFs only enqueue + +Since 3.0.0. Code: `worker_context_thread_main`, `owngil_context_thread_main`, +the request queue on `py_context_t` (`c_src/py_nif.c`, `c_src/py_nif.h`). + +## Situation + +Before 3.0.0 Python ran on dirty schedulers: a NIF took the GIL, swapped +in the context's thread state and executed the call. That pinned a dirty +scheduler for the whole call with a 30 s cap, gave numpy, torch and +tensorflow a different OS thread on every call (they keep per-thread +state and some refuse to work across threads), and needed a single-slot +request buffer that raced with concurrent callers. + +## Decision + +Every context owns one pthread that runs all of its Python. NIFs called +from Erlang processes allocate a `ctx_request_t`, append it to the +context's queue and return at once; the thread dequeues, executes with +the GIL, and sends `{py_result, Ref, Result}` back. The same thread +serves worker mode (main interpreter, shared GIL) and owngil mode (its +own interpreter and GIL). + +## Consequences + +- Stable thread affinity: a context's Python always runs on the same OS + thread, and no dirty scheduler is held during a call. +- Erlang processes never touch a context's Python objects; the lock and + ownership contract on `py_context_t` follows from this. +- Shutdown must join the thread. A thread stuck in a C call cannot be + joined, so the context is leaked on purpose rather than freed under a + running thread. +- Interrupts have to reach the thread from outside: `interrupt_mutex`, + `exec_thread_id` and `PyThreadState_SetAsyncExc`. +- One request at a time per context is a property of the design, not a + limitation to work around; parallelism comes from more contexts. diff --git a/docs/decisions/0002-callback-delivery-paths.md b/docs/decisions/0002-callback-delivery-paths.md new file mode 100644 index 0000000..db44aac --- /dev/null +++ b/docs/decisions/0002-callback-delivery-paths.md @@ -0,0 +1,43 @@ +# 0002: Three callback delivery paths, chosen by the calling thread + +Since 3.0.0. Code: `erlang_call_impl` in `c_src/py_callback.c`, +`c_src/py_thread_worker.c`, `src/py_thread_handler.erl`, the callback +handling in `src/py_context.erl`. + +## Situation + +`erlang.call` must work from the context thread, from a Python thread the +user started (executors, `threading.Thread`), and while the context +thread is already waiting for Erlang (nested calls). A single blocking +handler process deadlocks as soon as the callback calls Python again on +the same context. + +## Decision + +The path is chosen by who is calling: + +1. Suspension, on a context thread with suspension enabled: the call + raises `SuspensionRequired`, the context returns + `{suspended, ...}` to its process, which runs the function, serves + nested requests meanwhile, and resumes. Inspired by PyO3's model. +2. The context callback pipe, on a context thread with a handler + registered: the thread blocks on a pipe while the handler process + runs the function. +3. The thread worker, for every other Python thread: a per-thread worker + with its own pipe and handler process, coordinated by + `py_thread_handler`. + +All three answer with the same response body (`<<2, ETF>>` or +`<<1, Message>>`). + +## Consequences + +- Nested callbacks of any depth work on the suspension path because the + Erlang process is never blocked in a NIF while it waits. +- Three writers and two parsers of the response body must stay in step; + the protocols page lists them. +- A Python thread that calls Erlang is not a context thread and cannot + reach the calling context's Python objects; re-entrant calls into the + same owngil context from a spawned thread are not supported. +- Adding a mode means adding a path (isolated mode has a fourth: the + socket frame), not extending one of these. diff --git a/docs/decisions/0003-callback-results-as-etf.md b/docs/decisions/0003-callback-results-as-etf.md new file mode 100644 index 0000000..9e812bc --- /dev/null +++ b/docs/decisions/0003-callback-results-as-etf.md @@ -0,0 +1,32 @@ +# 0003: Callback results cross as external term format + +Since 4.0.0. Code: `handle_blocking_callback/3` and friends in +`src/py_context.erl`, `parse_callback_response` in `c_src/py_callback.c`, +`priv/_erlang_impl/_etf.py` for the child. + +## Situation + +Results of an Erlang callback used to reach Python as the Python repr of +the term, parsed with `ast.literal_eval`. Binaries with backslashes, +quotes or newlines produced unparseable literals and were handed over as +raw text, `[]` arrived as `''`, floats lost precision, and pids and +references went through a base64 marker that had to be decoded with an +unsafe `binary_to_term`. + +## Decision + +Results are encoded with `term_to_binary` and decoded by the same +`term_to_py` converter that call arguments use, with +`ERL_NIF_BIN2TERM_SAFE`. Pids and references cross as native objects. +The one visible change is accepted as breaking: an Erlang string +(`"abc"`) reaches Python as `[97, 98, 99]`, exactly as it does for +arguments; return a binary for a `str`. + +## Consequences + +- One type mapping in both directions, documented once + (`docs/type-conversion.md`, `c_src/py_convert.c`). +- The `__etf__:` marker is data, never re-interpreted; + `test_etf_decode_safe` guards that no atoms are minted. +- The child needs a Python ETF codec (`_etf.py`) that produces the same + terms as the C converter; keeping them in step is a maintenance duty. diff --git a/docs/decisions/0004-isolated-mode-child-process.md b/docs/decisions/0004-isolated-mode-child-process.md new file mode 100644 index 0000000..7d281bb --- /dev/null +++ b/docs/decisions/0004-isolated-mode-child-process.md @@ -0,0 +1,40 @@ +# 0004: Isolation is a child OS process over a Unix socket + +Since 5.0.0. Code: `src/py_isolated.erl`, `priv/py_isolated_child.py`, +`priv/_erlang_impl/_isolated.py`. + +## Situation + +Embedded modes cannot stop a Python call stuck in C, cannot bound its +memory or CPU, and a segfault in an extension kills the node. Running +untrusted or unknown Python code needs those guarantees, with the public +API unchanged so a pool can mix modes. + +## Decision + +An isolated context is one child process per context, started with +`open_port` so the VM reaps it, talking to its `py_context` process over +a Unix socket with the frame format of the callback pipe +(`<>`, body `<>`). Interrupts are a +signal to the child's main thread with a `SIGKILL` backstop; limits are +rlimits, cgroups v2 on Linux and an RSS watchdog on macOS; a crash +restarts the child within a budget. The child uses the standard asyncio +loop; there is no C code of its own in the VM beyond `os_kill`. + +Not chosen: a seccomp or Capsicum sandbox (a later hardening step, the +process boundary is the first one), a NIF-side sub-process pool, and a +new wire protocol (the existing frame and ETF conventions are enough). + +## Consequences + +- Everything crossing the socket is a term. NIF resources (channels, + native buffers, object references) do not cross; the API says so with + `{error, not_supported_in_isolated}`. +- The child can create atoms (`binary_to_term` is not called with + `safe` because handles and control terms need atoms); untrusted code + must not mint unbounded distinct atoms. +- Each call copies arguments and results through the socket; bulk data + needs shared memory (0006). +- Python state is lost on restart; the context stays usable. +- Platform code lives in the child launcher: parent-death signal + (`prctl` on Linux, `procctl` on FreeBSD), memory limits per OS. diff --git a/docs/decisions/0005-py-isolated-gen-statem.md b/docs/decisions/0005-py-isolated-gen-statem.md new file mode 100644 index 0000000..f8d838d --- /dev/null +++ b/docs/decisions/0005-py-isolated-gen-statem.md @@ -0,0 +1,39 @@ +# 0005: The isolated context process is a gen_statem + +Since 5.0.0. Code: `src/py_isolated.erl`, entered from `init/4` in `py_context` +with `gen_statem:enter_loop/5`. + +## Situation + +The embedded context process is a hand-written receive loop in +`py_context`. The isolated process has more to track: a request in +flight, requests to hold while the child restarts, a running loop with a +grace period, an interrupt with a kill backstop bound to one request id, +and callback processes whose nested requests must pass while others +wait. A first version as a receive loop mirrored `py_context` but every +wait needed its own selective receive and its own timer bookkeeping. + +## Decision + +`py_isolated` is a `gen_statem` (`handle_event_function`, state enter +calls) with states `idle`, `{busy, Id}`, `looping`, `stopping_loop` and +`{restarting, Reason}`. Requests that must wait are `postpone`d and +replayed by the behaviour in arrival order; timers are `state_timeout` +and named generic timeouts (`{timeout, kill}`) cancelled by the state +change that makes them moot. It is spawned with `proc_lib` so it keeps +the process identity and message protocol of `py_context`. + +Not chosen: sharing the receive loop with `py_context` (the two have +different failure models: a child can die and restart, a thread cannot), +or a `gen_server` with a state field (postpone and state timeouts would +have to be reimplemented). + +## Consequences + +- `sys:get_state/1` shows what a context is doing and `sys:trace/2` + prints every event; the state machines page is a transcription of the + callback module. +- Callers do not see the behaviour: messages and replies are those of + `py_context`. +- The two context processes are different code. A change to the message + protocol touches both. diff --git a/docs/decisions/0006-shared-memory-over-iommap.md b/docs/decisions/0006-shared-memory-over-iommap.md new file mode 100644 index 0000000..73f780a --- /dev/null +++ b/docs/decisions/0006-shared-memory-over-iommap.md @@ -0,0 +1,42 @@ +# 0006: Bulk data through iommap regions, handles as plain tuples + +Since 5.0.0. Code: `src/py_shm.erl`, `src/py_buffer.erl` (shared variant), +`priv/_erlang_impl/_shm.py`, the tagged-tuple case in `c_src/py_convert.c`. + +## Situation + +An isolated call copies its arguments and result through the socket: +1.3 ms per MB, 300 ms for 64 MB. Request bodies, arrays and model inputs +need a path that does not copy, and it must work the same in a pool that +mixes embedded and isolated contexts. + +## Decision + +A region is a file mapped `MAP_SHARED` by the VM through iommap +(`region_binary/3` gives a refcounted binary with no copy) and by any +interpreter through `mmap`. Its handle is the plain term +`{'$py_shm', Id, Path, Size}`, so it travels inside any argument or +result with no special encoding and becomes a `SharedMemory` on arrival +in every mode (the C converter and the child both call `_shm.from_term`). +A shared `py_buffer` is a region used as a ring, with flow control +through registered callbacks (`_py_buffer_wait`, `_py_buffer_consumed`), +the mechanism channels already use, so no new control frames exist. +iommap is optional: `py_shm:new/1` returns `{error, iommap_not_available}` +without it. + +Not chosen: passing the region's fd (iommap exposes none; a path in a +0700 directory is enough), a NIF of our own for mapping, and channels +over shared memory (small terms stay on the socket). + +## Consequences + +- Python-produced data is zero-copy both ways; Erlang-produced data + costs one `pwrite` because a binary cannot be written in place. +- Sharing memory weakens isolation for that region only; read-only + handles keep a callee from writing. Sealing and syscall filtering are + separate work. +- In embedded contexts a shared buffer costs a callback round trip per + blocking read where the native buffer costs a pointer; the guide says + when to use which. +- The region file must never be truncated (`SIGBUS`); sizes are fixed at + creation and verified with `fstat` before mapping. diff --git a/docs/decisions/0007-remove-legacy-execution-paths.md b/docs/decisions/0007-remove-legacy-execution-paths.md new file mode 100644 index 0000000..5c7372c --- /dev/null +++ b/docs/decisions/0007-remove-legacy-execution-paths.md @@ -0,0 +1,31 @@ +# 0007: One execution path per mode; the legacy API is removed + +Since 5.0.0. Code: the removal in `c_src/py_nif.c`, `c_src/py_exec.c`, +`c_src/py_callback.c`, `src/py_nif.erl` (PR #75). + +## Situation + +After 0001 every context had a thread, but the code still carried the +paths from before: an executor thread with a worker pool, blocking NIF +variants that ran Python on dirty schedulers, suspended-state resources +for a resume protocol nothing used, and `py_nif` stubs for all of it. +Roughly 4 500 lines were reachable only from suites or from nothing, and +every reader had to work out which of two paths was live. + +## Decision + +Remove them. A context created today has exactly one path per mode: +the queue and context thread for worker and owngil, the socket for +isolated. NIFs that needed a thread now answer +`{error, context_has_no_thread}` instead of falling back to a scheduler. +Because public functions disappeared, the release that carries this is a +major version (5.0.0), not a minor one. + +## Consequences + +- `docs/code-map.md` can say "live" for everything in `src/` and + `c_src/` except the test helpers, and mean it. +- There is no fallback when a context has no thread; that state is a + bug, and it is reported as one. +- Anyone on the removed functions upgrades through the changelog's + Removed section. diff --git a/docs/decisions/0008-pipe-io-rules.md b/docs/decisions/0008-pipe-io-rules.md new file mode 100644 index 0000000..da9fac3 --- /dev/null +++ b/docs/decisions/0008-pipe-io-rules.md @@ -0,0 +1,31 @@ +# 0008: Pipe I/O is non-blocking, deadlined and waited with poll + +Since 3.1.0 (deadlines), 5.0.0 (poll). Code: `read_with_timeout` and +`write_all_with_deadline` in `c_src/py_nif.h`, the pipe setup in +`c_src/py_thread_worker.c` and `c_src/py_callback.c`. + +## Situation + +Callback responses are written by Erlang processes on dirty I/O +schedulers into pipes read by Python threads. A blocking write to a +stalled reader pinned a dirty scheduler for good; a short read or write +left the framed protocol out of phase with no way back. Later, a CI run +with more than 1024 open files showed that `select()` cannot watch such a +descriptor at all, and the first thread worker created past that point +took every later thread callback down with it. + +## Decision + +Write ends are `O_NONBLOCK`; every write is `write_all_with_deadline` +and every read `read_with_timeout`, both waiting with `poll()`. A +partial frame is never recovered in band: the thread worker is poisoned +and replaced, and the context pipe is closed only when its thread has +been joined. The coordinator reports a failed ready signal instead of +leaving Python to time out. + +## Consequences + +- No scheduler thread waits on Python without a bound. +- A desynchronised pipe costs one worker, not a hang. +- `select()` and `` do not appear in the NIF; a review + that sees them come back should ask why. diff --git a/docs/decisions/overview.md b/docs/decisions/overview.md new file mode 100644 index 0000000..0d1f7eb --- /dev/null +++ b/docs/decisions/overview.md @@ -0,0 +1,18 @@ +# Decision records + +Why the code is the way it is, one decision per file, in the order they +were taken. Read the record before changing what it decided; if the +reasons no longer hold, write a new record that supersedes it rather than +editing the old one. Each record has the same four parts: the situation, +what was decided, what it costs, and where the code is. + +| # | Decision | Since | +|---|---|---| +| [0001](0001-one-thread-per-context.md) | One pthread per context, NIFs only enqueue | 3.0.0 | +| [0002](0002-callback-delivery-paths.md) | Three callback delivery paths, chosen by the calling thread | 3.0.0 | +| [0003](0003-callback-results-as-etf.md) | Callback results cross as external term format | 4.0.0 | +| [0004](0004-isolated-mode-child-process.md) | Isolation is a child OS process over a Unix socket | 5.0.0 | +| [0005](0005-py-isolated-gen-statem.md) | The isolated context process is a gen_statem | 5.0.0 | +| [0006](0006-shared-memory-over-iommap.md) | Bulk data through iommap regions, handles as plain tuples | 5.0.0 | +| [0007](0007-remove-legacy-execution-paths.md) | One execution path per mode; the legacy API is removed | 5.0.0 | +| [0008](0008-pipe-io-rules.md) | Pipe I/O is non-blocking, deadlined and waited with poll | 3.1.0, 5.0.0 | diff --git a/rebar.config b/rebar.config index 7be76e5..ddae020 100644 --- a/rebar.config +++ b/rebar.config @@ -82,6 +82,15 @@ <<"docs/protocols.md">>, <<"docs/state-machines.md">>, <<"docs/contributing.md">>, + <<"docs/decisions/overview.md">>, + <<"docs/decisions/0001-one-thread-per-context.md">>, + <<"docs/decisions/0002-callback-delivery-paths.md">>, + <<"docs/decisions/0003-callback-results-as-etf.md">>, + <<"docs/decisions/0004-isolated-mode-child-process.md">>, + <<"docs/decisions/0005-py-isolated-gen-statem.md">>, + <<"docs/decisions/0006-shared-memory-over-iommap.md">>, + <<"docs/decisions/0007-remove-legacy-execution-paths.md">>, + <<"docs/decisions/0008-pipe-io-rules.md">>, <<"docs/preload.md">>, <<"docs/owngil_internals.md">>, <<"docs/event_loop_architecture.md">> @@ -126,6 +135,17 @@ <<"docs/preload.md">>, <<"docs/owngil_internals.md">>, <<"docs/event_loop_architecture.md">> + ]}, + {<<"Decisions">>, [ + <<"docs/decisions/overview.md">>, + <<"docs/decisions/0001-one-thread-per-context.md">>, + <<"docs/decisions/0002-callback-delivery-paths.md">>, + <<"docs/decisions/0003-callback-results-as-etf.md">>, + <<"docs/decisions/0004-isolated-mode-child-process.md">>, + <<"docs/decisions/0005-py-isolated-gen-statem.md">>, + <<"docs/decisions/0006-shared-memory-over-iommap.md">>, + <<"docs/decisions/0007-remove-legacy-execution-paths.md">>, + <<"docs/decisions/0008-pipe-io-rules.md">> ]} ]} ]}.