Skip to content

[NOGIL] Fix Producer.close() races with concurrent calls and with itself#2313

Open
Ojasva Jain (ojasvajain) wants to merge 4 commits into
dev_thread_free_supportfrom
dev_producer_no_gil
Open

[NOGIL] Fix Producer.close() races with concurrent calls and with itself#2313
Ojasva Jain (ojasvajain) wants to merge 4 commits into
dev_thread_free_supportfrom
dev_producer_no_gil

Conversation

@ojasvajain

Copy link
Copy Markdown
Member

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.

What

Checklist

  • Contains customer facing changes? Including API/behavior changes
  • Did you add sufficient unit test and/or integration test coverage for this PR?
    • If not, please explain why it is not required

References

JIRA:

Test & Review

Open questions / Follow-ups

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.
@confluent-cla-assistant

Copy link
Copy Markdown

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

@sonarqube-confluent

Copy link
Copy Markdown

Quality Gate failed Quality Gate failed

Failed conditions
12.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant