Repository navigation
Closes #1472: Implement in-memory config backend for config overrides in context manager - #1503
beamerblvd wants to merge 2 commits into
Conversation
…errides in context manager This implements libgit2#1472. When `DefaultConfig` and `RepositoryConfig` are constructed, they now construct an in-memory config backend with level `ConfigLevel.APP` that, initially, is unused. When instances of these classes are entered as a context manager, that in-memory backend is given sole write priority. When instances of these classes are exited as a context manager, that in-memory backend in cleared, and the default write priority is restored.
|
Kimi's review... Blocker: PyPy test-suite hang (4 CI jobs × 6h timeouts)gh pr checks 1503 shows exactly four failed jobs — linux-amd-glibc, linux-arm-glibc, macos-intel, windows-x64 — each canceled after ~6 hours. Those four are precisely the platforms that run PyPy tests (pp311); every platform without PyPy tests passes, and master's CI is green. Digging into the job logs, all four die in the same place: the pp311 session completes test_filter.py::test_filter_buffered and then produces no output for 6 hours (an orphaned python process is killed at cancellation). All 7 CPython sessions and all config tests, including the new ones, pass on PyPy before the hang. I reproduced it locally with the exact CI build (portable pypy3.11-v8.0.0-linux64, libgit2 1.9.7):
So: the config tests create garbage that, once PyPy's collector gets to it during the fixture's explicit gc.collect(), deadlocks — plausibly where PyPy runs weakref callbacks/finalizers against the PR's new CFFI objects (ffi.new_handled backends kept alive from C, 16 new extern "Python" callbacks). pygit2's own weakref callback at pygit2/filter.py:139 is in the immediate neighborhood. The root cause needs the author's investigation; the recipe above reproduces it in ~60s. Whatever the fix, the PR cannot merge with PyPy hanging — and I'd suggest adding -o faulthandler_timeout=120 (or pytest-timeout) to the cibuildwheel test command so future hangs dump a stack instead of burning 6 hours. Bug: get returns the first value of a multivarpygit2/config.py:1577 returns self._read_data[key][0]. libgit2's file backend, libgit2's own in-memory backend (config_mem.c), and git CLI semantics all return the last value (git_config_list_get keeps the last appended entry; verified empirically: file backend config['x.y'] → 'last', the PR's APP backend → 'first'). Inside an override context, plain reads and get_bool/get_int silently disagree with what the same values would yield from a file. One-line fix: index [-1]. Bug: nested with config: silently writes through to diskenter/exit (pygit2/config.py:828-867) have no re-entrancy tracking. Verified behavior: with cfg: # write order → (APP,)
cfg['a.b'] = 'outer1'
with cfg: # no-op
cfg['a.c'] = 'inner'
# inner __exit__ already restored write order to (LOCAL,) and cleared the backend
cfg['a.d'] = 'outer2' # ← persisted to .git/configa.d landed in .git/config while the user still believes they're in an override context; the outer context's earlier overrides were also erased by the inner exit. A depth counter (only the outermost exit clears/restores), or explicitly rejecting nested entry, would fix it. At minimum this needs documenting since the current docstrings advertise re-entry. Minor issues
|
- In-memory backend now returns last entry in a multivar instead of first entry to be consistent with Git CLI and stock libgit2 backends. - Nested entry into config context manager is now explicitly disallowed (in code and in documentation). This was never meant to be supported. - Added missing acquisition of write and read locks in `_config_memory_backend_set` and `_config_memory_backend_get`. - Fixed mismatched backtick in `_set_write_order` docstring. - Config entry name case is now preserved even while it is matched case-insensitively. - Fixed type of first argument for FFI stub of `GitConfigIteratorC.next`.
|
I have implemented most of your feedback. I'm going to have to spend more time on the rest of your feedback, but I wanted to go ahead and push some changes to get another build run and additional feedback if you have it. Here's what's left:
|
|
Thanks @beamerblvd First rebase over master, one build issue has been fixed in master (excluding recent versions of setuptools, which has a regression). Kimi is happy with the changes. It remains the PyPy hang, here some more info about it (and some new feedback)... Blocker: the PyPy hang — two separate deadlocksI reproduced both with portable PyPy 8.0.1 (same as CI) + libgit2 1.9.7. pytest test/test_config.py test/test_filter.py hangs every time; each test file alone passes. Deadlock 1 — abandoned iterator pins the write lock (faulthandler stack: config.py write_lock ← _config_memory_backend_set ← setitem). multivar_generator() holds read_lock() (which acquires _write_lock on first reader) across its yields. The new test's early-breaked for entry in config: loop abandons the generator, so _write_lock stays held; the next config[...] = ... self-deadlocks on the same thread. CPython survives only because refcounting closes the generator promptly — on PyPy it survives until a GC cycle that never comes in time. Fix (validated): snapshot under the lock, then yield unlocked: with self.read_lock():
items = [(k, v) for k, vs in self._read_data.items() for v in vs]
for key, value in items:
yield key, valueThis is also better semantics — an iterator shouldn't block writers until it's exhausted/closed. Deadlock 2 — the _weak_config weakref in a cycle wedges PyPy's GC (faulthandler stack: _weakrefset._remove ← gc.collect() in the filter fixture). With deadlock 1 fixed, the hang moves into gc.collect() during test_filter.py fixture teardown, inside threading._dangling's WeakSet callback — the frame varies by run (WeakSet callback, zipfile.del) because it's the collector that's wedged. I bisected it to self._weak_config = weakref.ref(config): config → _backend → weakref → config is a cycle, and PyPy's cycle collector deadlocks finalizing it once cffi handles/finalizers join the graph. Replacing the weakref with a strong reference eliminates it (a plain weakref cycle doesn't trigger it; the cffi ingredient is required). Full-suite green on PyPy with both fixes: 675 passed, 10 skipped. The author should design the real fix — my worktree uses lambda: config just to prove the point. Options: hold the config strongly and accept the collectable cycle (update the ReferenceType annotation and _do_on_snapshot_completion accordingly), or eliminate the back-reference (e.g. store exceptions on the backend and have Config._check_error consult it). Worth filing a PyPy bug too; I can help minimize it if wanted. Other issues found this round
|
This implements #1472. When
DefaultConfigandRepositoryConfigare constructed, they now construct an in-memory config backend with levelConfigLevel.APPthat, initially, is unused. When instances of these classes are entered as a context manager, that in-memory backend is given sole write priority. When instances of these classes are exited as a context manager, that in-memory backend in cleared, and the default write priority is restored.This pull request is the final subset of the changes in #1473 and a replacement for that pull request, which I will now close.