Fix the UniversalDetector uchardet_t handle lifecycle

[wbarnha]

Follow-up to #55 / #68. Now rebased onto master — #68 has merged, so this gets the full CI matrix.

These are all pre-existing bugs, not free-threading ones. They landed after #68 because every guard added here is a check-then-act on _ud, which is only atomic on a free-threaded build thanks to the critical sections #68 introduced.

The leak

The uchardet_t handle was only ever released by an explicit close(). There is no __dealloc__, so a detector that was simply dropped leaked it — and dropping is the documented pattern, since reading result finalizes detection on its own precisely so callers can stop without closing.

build	20,000 detectors dropped without close()
before	RSS +380 MB (19,458 B/detector)
this PR	RSS +0 B

Four inseparable parts

1. __dealloc__ releases the handle.
2. close() and feed()'s error path clear _ud after uchardet_delete(). This is not optional hygiene — tp_dealloc still runs for the object afterwards, so __dealloc__ without these assignments makes every explicitly closed detector a double free. Verified: a build with __dealloc__ and no NULLing segfaults (rc=139) on the first close-then-drop, where the full change exits 0.
3. Allocation moves from __init__ to __cinit__, which runs exactly once and cannot be re-entered from Python. This also fixes a pre-existing segfault: a detector built without running __init__ — via __new__, or a subclass that doesn't call super().__init__() — had _ud NULL and dereferenced it on the first feed().
4. Every remaining uchardet_* call site is guarded on _ud. Operating on a released detector stays a silent no-op rather than becoming an error: close() has to remain idempotent, and feed()/reset() were already no-ops once _closed was set.

Why __init__ still does work

An earlier draft made __init__ empty, since __cinit__ now owns allocation. That was a silent wrong-answer regression: d.__init__() on a live detector would concatenate the next feed() onto the previous stream and report a bogus mixed-encoding label (KOI8-R where a fresh detector says WINDOWS-1251) with no exception and no warning.

So __init__ resets the live handle instead, and allocates a new one only when the previous was closed — preserving the original semantics without reintroducing the leak. All three re-init paths (live, finalized, closed) return byte-identical results to a plain fresh detector.

Out of memory, uchardet took the interpreter down with it

uchardet is C++ and allocates with plain new, so allocation failure throws std::bad_alloc rather than returning NULL — which means uchardet's own out-of-memory checks are dead code:

mCharSetProbers[0] = new nsMBCSGroupProber(mLanguageFilter);
if (nsnull == mCharSetProbers[0])
  return NS_ERROR_OUT_OF_MEMORY;      /* never taken */

The exception then unwound out of the extension into CPython's C frames. That is undefined behaviour, and in practice std::terminate():

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc

Four entry points reach a new: uchardet_new() (new HandleUniversalDetector), uchardet_handle_data() (the group probers), uchardet_reset() (nsMBCSGroupProber's code-point buffers) and uchardet_data_end() (reporting candidates into a std::vector). All four are now declared except +, so Cython catches and translates to MemoryError. uchardet_delete() deliberately is not: it runs a destructor, and it is called from __dealloc__ where nothing could be propagated anyway. The NULL checks stay for any implementation that really does return NULL, including a system libuchardet built with -fno-exceptions.

close() had to release the handle in a finally

detect_with_confidence() gets a try/finally here for one reason: assigning uchardet_get_encoding() to a bytes is a PyBytes_FromString, which can raise MemoryError and jump straight to Cython's error label, skipping the uchardet_delete() underneath it. (Before this PR that function deleted the handle separately at each exit, so the error label leaked it.)

close() reaches the same conversion through _finalize() → _read_candidate(), and being cdef void does not make those safe: since Cython 3 a void cdef function propagates exceptions via a PyErr_Occurred() check at the call site.

/* generated from master's _cchardet.pyx; vtable dispatch elided */
...->_finalize(__pyx_v_self); if (unlikely(PyErr_Occurred())) __PYX_ERR(0, 150, __pyx_L4_error)

That check jumps past both the uchardet_delete() and the self._closed = 1 underneath it, so a failed close() was simply undone — the detector stayed open, still holding its handle, and reading result afterwards silently re-finalized the stream and answered as if close() had never been called.

Tests

src/tests/test_lifecycle.py, 14 tests. Against the unfixed build the file segfaults on the __new__ case and fails the leak assertion; the two re-init tests pass there by construction and exist to keep that behaviour from regressing.

The out-of-memory paths need the failure injected, and the two levers are not interchangeable:

lever	reaches	used for
RLIMIT_AS + mmap the address space away	uchardet's C++ new	the except + translation
_testcapi.set_nomemory()	PyMem_* / PyObject_* only	the PyBytes_FromString inside close()

Real memory pressure cannot reach the close() bug: uchardet_data_end()'s allocations are small and keep being served from the heap free list long after the address space is exhausted, so close() simply succeeds. set_nomemory() is precise and leaves C++ new alone, so the failure lands exactly where the bug lives.

Each fix was verified by reverting only itself:

reverted	result
except +	the allocation test fails 3/3, rc=-6, std::bad_alloc in stderr
close()'s finally	detector reports open after a failed close(); 3,200 held detectors leak ~19 KB each, vs ~87 B with the fix

That second number is the point of holding them: reporting "closed" only shows _closed was set, not that uchardet_delete() ran. Nothing is dropped, so __dealloc__ never runs and close() is the only thing that can have released a handle.

The allocation-failure test is deliberately biased towards skipping. Starving a process that hard makes it fragile in ways that have nothing to do with uchardet — it first went red on 3.10/3.13/3.14 with cannot allocate memory for thread-local data from the dynamic loader — so the verdict is written to a file the instant it is known rather than printed at the end where a later death would erase it, and only the std::bad_alloc signature counts as a failure. Anything else means the experiment did not run on that runner.

Both RSS tests run in a subprocess and are Linux-only — ru_maxrss is KB on Linux but bytes on macOS, and the leak is platform-independent, so there's no reason to encode that quirk into a CI gate. The two _testcapi tests skip where that module is absent (PyPy, stripped builds).

153 collected; 151 passed, 2 skipped on every interpreter in the matrix — 3.10, 3.11, 3.12, 3.13, 3.13t, 3.14, 3.14t — plus both -Dsystem-uchardet jobs. The allocation-failure test is the one that varies: it ran on 3.10 and skipped on 3.13t in the same run, which is the intended behaviour rather than a flake.

_cchardet.pyi (added by #71) stays accurate: __cinit__/__dealloc__ are not part of the type surface and no public signature changed. Verified with mypy clean on the rebased tree.