Skip to content

Commit 1fb8714

Browse files
authored
Add the decision records (#81)
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.
1 parent 8e42d42 commit 1fb8714

12 files changed

Lines changed: 336 additions & 1 deletion

‎docs/architecture.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,8 @@ same context are dispatched immediately because they come from that process.
129129

130130
Every message and frame, with the rules for changing them, is in
131131
[protocols](protocols.md); the states each process and thread moves through
132-
are in [state machines](state-machines.md).
132+
are in [state machines](state-machines.md); the reasons behind the design
133+
are in the [decision records](decisions/overview.md).
133134

134135
## asyncio
135136

‎docs/contributing.md‎

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

0 commit comments

Comments
 (0)