Skip to content

Remove dead Admin#finalizer, fix cascading admin spec failures #842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mensfeld merged 1 commit into from
Mar 24, 2026
Merged

Remove dead Admin#finalizer, fix cascading admin spec failures #842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
# Rdkafka Changelog

## 0.25.3 (Unreleased)
- [Fix] Register `ObjectSpace.define_finalizer` in `Rdkafka::Consumer` to prevent segfaults when a consumer is GC'd without being explicitly closed (jturkel).
- [Fix] Remove dead `#finalizer` instance methods from `Consumer` and `Admin` that could never work as GC finalizers.
- [Fix] Prevent cascading test failures in admin specs when a single handle leaks into the registry.

## 0.25.2 (2026-02-27)
- [Feature] Extend `Rdkafka::RdkafkaError` with `instance_name` attribute containing the `rd_kafka_name` for tying errors back to specific native Kafka instances (#181).

Expand Down
6 changes: 0 additions & 6 deletions lib/rdkafka/admin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,6 @@ def events_poll_nb_each
end
end

# @return [Proc] finalizer proc for closing the admin
# @private
def finalizer
->(_) { close }
end

# Performs the metadata request using admin
#
# @param topic_name [String, nil] metadat about particular topic or all if nil
Expand Down
27 changes: 19 additions & 8 deletions spec/lib/rdkafka/admin_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,26 @@
let(:permission_type) { Rdkafka::Bindings::RD_KAFKA_ACL_PERMISSION_TYPE_ALLOW }
let(:admin) { config.admin }

after do
# Registry should always end up being empty
expect(Rdkafka::Admin::CreateTopicHandle::REGISTRY).to be_empty
expect(Rdkafka::Admin::CreatePartitionsHandle::REGISTRY).to be_empty
expect(Rdkafka::Admin::DescribeAclHandle::REGISTRY).to be_empty
expect(Rdkafka::Admin::CreateAclHandle::REGISTRY).to be_empty
expect(Rdkafka::Admin::DeleteAclHandle::REGISTRY).to be_empty
expect(Rdkafka::Admin::ListOffsetsHandle::REGISTRY).to be_empty
around do |example|
example.run
ensure
# Registry should always end up being empty after each test.
# We check and then clear to prevent cascading failures when a single test
# leaves a leaked handle (e.g. due to a timeout or broker error).
registry_leaks = [
Rdkafka::Admin::CreateTopicHandle,
Rdkafka::Admin::CreatePartitionsHandle,
Rdkafka::Admin::DescribeAclHandle,
Rdkafka::Admin::CreateAclHandle,
Rdkafka::Admin::DeleteAclHandle,
Rdkafka::Admin::ListOffsetsHandle
].reject { |handle_class| handle_class::REGISTRY.empty? }

registry_leaks.each { |handle_class| handle_class::REGISTRY.clear }

admin.close

expect(registry_leaks).to be_empty, "Leaked handles in: #{registry_leaks.map(&:name).join(", ")}"
Comment on lines +28 to +41
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these handle classes appear to share the same REGISTRY constant via inheritance from Rdkafka::AbstractHandle (none of the admin handle subclasses define their own REGISTRY). As a result, registry_leaks will either be empty or contain all listed classes, and the failure message will be misleading about which handle type actually leaked. Consider checking/clearing Rdkafka::AbstractHandle::REGISTRY once and deriving leaked types from REGISTRY.values.map(&:class).uniq (or alternatively defining per-handle REGISTRY constants if separation is intended).

Suggested change
registry_leaks = [
Rdkafka::Admin::CreateTopicHandle,
Rdkafka::Admin::CreatePartitionsHandle,
Rdkafka::Admin::DescribeAclHandle,
Rdkafka::Admin::CreateAclHandle,
Rdkafka::Admin::DeleteAclHandle,
Rdkafka::Admin::ListOffsetsHandle
].reject { |handle_class| handle_class::REGISTRY.empty? }
registry_leaks.each { |handle_class| handle_class::REGISTRY.clear }
admin.close
expect(registry_leaks).to be_empty, "Leaked handles in: #{registry_leaks.map(&:name).join(", ")}"
admin_handle_classes = [
Rdkafka::Admin::CreateTopicHandle,
Rdkafka::Admin::CreatePartitionsHandle,
Rdkafka::Admin::DescribeAclHandle,
Rdkafka::Admin::CreateAclHandle,
Rdkafka::Admin::DeleteAclHandle,
Rdkafka::Admin::ListOffsetsHandle
]
registry = Rdkafka::AbstractHandle::REGISTRY
leaked_handle_classes =
registry.values.map(&:class).uniq & admin_handle_classes
registry.clear
admin.close
expect(leaked_handle_classes).to be_empty,
"Leaked handles in: #{leaked_handle_classes.map(&:name).join(", ")}"

Copilot uses AI. Check for mistakes.
end

describe "#describe_errors" do
Expand Down
Loading