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">> ]} ]} ]}.