[NOGIL] Guard AdminClient against concurrent close() vs method-call races - #2317
Open
Ojasva Jain (ojasvajain) wants to merge 9 commits into
Open
[NOGIL] Guard AdminClient against concurrent close() vs method-call races#2317Ojasva Jain (ojasvajain) wants to merge 9 commits into
Ojasva Jain (ojasvajain) wants to merge 9 commits into
Conversation
* Add CI verification jobs for free-threaded Python 3.14t
- Add a Semaphore block running source package verification and
integration tests on CPython 3.14t (classic and consumer group
protocols). cimpl does not declare free-threading support yet, so
importing it re-enables the GIL: these jobs validate the 3.14t
toolchain and packaging until that declaration ships.
- Skip the CI-only orjson install on free-threaded interpreters: no
free-threaded orjson wheels exist and the source build would fail;
the stdlib JSON fallback path stays covered.
- Add a module-scoped autouse fixture (defined on free-threaded builds
only) that warns when the GIL is re-enabled around a test module.
Hard asserts are staged behind TODO FTS markers, to be enabled in the
same PR that declares Py_MOD_GIL_NOT_USED.
Interpreter detection follows the free-threading HOWTO:
https://docs.python.org/3/howto/free-threading-python.html
* Handle deps without free-threaded wheels in test setup
On free-threaded (no-GIL) builds, the rules and json-fast extras'
compiled deps (tink, google-re2, grpcio; orjson) ship no free-threaded
wheels and fail to build from source, so:
- Add requirements-tests-install-nogil.txt, a variant of
requirements-tests-install.txt without those extras, and install it
from source-package-verification.sh when the interpreter is
free-threaded (detected via Py_GIL_DISABLED).
- Exclude the schema_registry test modules that import tink/celpy/orjson
at the top of the file from collection on free-threaded builds only;
on regular builds a missing dep stays a loud collection error rather
than a silent skip. Plain serdes coverage recovery is marked as a
TODO NOGIL follow-up.
* Style fixes
…usions (#2309) * [NOGIL] Split schema_registry tests to narrow free-threaded test exclusions Each of test_avro_serdes.py, test_config.py, test_json_serdes.py, and test_proto_serdes.py is split into a plain file (no rules/encryption dependency) and a _rules file (CEL/encryption/JSONata-dependent tests). This lets the plain tests run on free-threaded (3.14t) builds, where tink/celpy/orjson have no free-threaded wheels, while only the _rules files stay excluded via conftest.py's collect_ignore. * Fix isort/black formatting in split schema_registry tests
…stry tests from free-threaded collection test_azure_aead.py, test_azure_client.py, test_azure_driver.py, and test_encrypt_executor.py import azure/tink unconditionally at module level but were missing from conftest.py's free-threaded collect_ignore list, causing collection errors on 3.14t CI. These tests were added to master after the original NOGIL schema_registry exclusion list (#2309) landed, so they weren't accounted for; rebasing onto master surfaced the gap.
Producer.close() previously raced with concurrent produce()/poll()/ flush()/produce_batch()/transaction calls and with itself when called from multiple threads, both leading to use-after-free/double-free on the underlying rd_kafka_t handle. Adds an active_calls/closing guard (Handle_enter_rk_use/Handle_exit_rk_use) so every method that touches self->rk registers itself before use, and close() drains in-flight calls before tearing down; a CAS on `closing` ensures only one concurrent close() call performs the actual teardown, with losing callers waiting for it to finish rather than racing it. Adds tests/parallel/test_producer_close_race.py covering each affected method racing close(), close() racing itself, and close()'s blocking behavior. Uses pytest-forked (POSIX only) so a regression segfault fails only that test. Integration tests against a real broker are still pending.
…for Producer close()/transaction races
…aces Adds the same active_calls/closing gate Producer.close() already has to AdminClient: every Admin-owned method now enters/exits the gate around self->rk, and Admin_exit() uses the same CAS-based teardown protocol as Producer_close(), draining in-flight calls before destroying the client. Also closes a gap in the functions shared across Producer/Consumer/Admin (list_topics, list_groups, set_sasl_credentials), which were previously unguarded entirely -- they're now gated for Producer/Admin (Consumer gating is tracked separately). Adds concurrency unit tests (subprocess-isolated, no broker) and real-broker integration tests covering exit-races-in-flight-call, exit-races-itself, and shared-instance multi-API usage across threads.
Ojasva Jain (ojasvajain)
requested review from
a team and
Matthew Seal (MSeal)
as code owners
July 29, 2026 09:14
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
|
airlock-confluentinc
Bot
force-pushed
the
dev_producer_no_gil
branch
2 times, most recently
from
July 31, 2026 08:23
b0425e3 to
7d63d3b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An AdminClient instance can be shared across multiple threads — this is a common, previously-supported pattern. One long-standing race in this pattern: one thread destroys the Admin client (setting rk to NULL) while another thread is concurrently calling a librdkafka API on that same rk. This race has always existed, but on free-threaded builds the window widens significantly since there's no GIL to serialize the two threads.
Change
To close this race, every Admin API is now gated using active_calls/closing atomic variables. A CAS (compare-and-swap) construct has been added to exit() to correctly handle concurrent exit() calls. This mirrors the gating already implemented for Producer.
Other changes
APIs shared across clients (e.g. list_topics, set_sasl_credentials) were previously left unguarded. They are now gated for Producer and Admin. Gating is not needed for Consumer yet, since concurrent usage of a Consumer's rk will be restricted in a future PR.
Adds concurrency-related unit and integration test cases.
What
Checklist
References
JIRA:
Test & Review
Open questions / Follow-ups