Fix the intermittent segfault issue due to GC in Admin tests #2129
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a segmentation fault issue in Admin tests by disabling Python's garbage collector during callback execution in librdkafka's background threads. The fix prevents AdminClient objects from being destroyed by the garbage collector while running in librdkafka's threads, which is forbidden by the library.
Key Changes:
- Added garbage collector disabling/enabling logic around callback execution in multiple admin result handlers
- Removed extraneous blank lines for code cleanup
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import gc | ||
| gc_was_enabled = gc.isenabled() | ||
| gc.disable() |
There was a problem hiding this comment.
Importing gc inside each callback function creates unnecessary overhead when these callbacks are invoked repeatedly. Consider moving the import gc statement to the module level at the top of the file to avoid repeated import lookups.
| # Disable GC during callback to prevent AdminClient destruction from librdkafka thread. | ||
| # This callback runs in librdkafka's background thread, and if GC runs here, it may | ||
| # try to destroy AdminClient objects, which librdkafka forbids from its own threads. |
There was a problem hiding this comment.
This detailed explanation of why GC is disabled should be added to all other callback methods that use the same pattern. Currently, only _make_consumer_group_offsets_result has this documentation, while _make_topics_result, _make_resource_result, _make_consumer_groups_result, _make_acls_result, _make_futmap_result_from_list, and _make_futmap_result lack explanation for the same GC disabling logic.
There was a problem hiding this comment.
This is a good point by copilot. Maybe we put to desription at the import and a small comment at disable saying see import comment for preventing null resource segfault errors?
| """ | ||
| try: | ||
| import gc | ||
|
|
There was a problem hiding this comment.
[nitpick] The blank line at 237 between the import statement and the explanatory comment is inconsistent with the formatting in other methods where the GC setup code follows immediately after the import. Consider removing this blank line for consistency.
fangnx
changed the title
| # Disable GC during callback to prevent AdminClient destruction from librdkafka thread. | ||
| # This callback runs in librdkafka's background thread, and if GC runs here, it may | ||
| # try to destroy AdminClient objects, which librdkafka forbids from its own threads. |
There was a problem hiding this comment.
This is a good point by copilot. Maybe we put to desription at the import and a small comment at disable saying see import comment for preventing null resource segfault errors?
airlock-confluentinc
bot
force-pushed
the
admin-segfault
branch
from
629643e to
6cafc2c
Compare
airlock-confluentinc
bot
force-pushed
the
admin-segfault
branch
from
1246593 to
f30945f
Compare
|
What
Bug:
test_list_consumer_group_offsets_api) finishes but after that the callback function (_make_consumer_group_offsets_result), running in the librdkafka's background, triggers garbage collection. When librdkafka detects thatrd_kafka_destroyis invoked from its own thread (https://github.com/confluentinc/librdkafka/blob/616fb6e8074e82e0302f1c7d2896d7e577f8eb9b/src/rdkafka.c#L1115), it sends the ABORT signalFIx:
gc.disable()in tests that make fire-and-forget callsChecklist
References
JIRA: https://confluentinc.atlassian.net/browse/NONJAVACLI-4105
Test & Review
Open questions / Follow-ups