Skip to content

fix(python): read consumer metadata without the consumer lock - #3888

Merged
hubcio merged 14 commits into
apache:masterfrom
ethanlin01x:fix/python-consumer-metadata-deadlock
Aug 26, 2026
Merged

fix(python): read consumer metadata without the consumer lock#3888
hubcio merged 14 commits into
apache:masterfrom
ethanlin01x:fix/python-consumer-metadata-deadlock

Conversation

@ethanlin01x

@ethanlin01x ethanlin01x commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR address?

Relates to #3776 (Found while reviewing)

Rationale

The synchronous getters on the Python IggyConsumer took the consumer mutex with blocking_lock() while holding the GIL, and consume_messages holds that mutex for the whole consumption run. Reading an attribute during consumption hung the interpreter; reading one from a callback panicked inside the Tokio runtime. Neither is recoverable from Python.

consume = consumer.consume_messages(handle, shutdown_event)
print(consumer.name())   # never returns

What changed?

None of those getters need exclusive access.

name, stream and topic never change after construction, so the Python wrapper snapshots them. The partition id and offsets do change, but already live behind Arcs in the Rust SDK, which now hands them out as IggyConsumerState via IggyConsumer::state(). The wrapper keeps a clone and reads them as atomic loads, never taking the lock.

Additive on the Rust side. On the Python side stream() and topic() now return the identifier directly instead of a PyResult.

Local Execution

  • Passed
  • Pre-commit hooks ran

AI Usage

  1. Which tools? Claude
  2. Scope of usage? help implement and write PR description
  3. How did you verify the generated code works correctly? Ran the Python test suite against a real server
  4. Can you explain every line of the code if asked? Yes, all the changes are checked by the human.

@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.22705% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.05%. Comparing base (5068a96) to head (fb031bd).

Files with missing lines Patch % Lines
core/sdk/src/clients/consumer.rs 76.10% 35 Missing and 3 partials ⚠️
foreign/python/src/consumer.rs 87.50% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3888      +/-   ##
============================================
+ Coverage     84.04%   84.05%   +0.01%     
  Complexity     1358     1358              
============================================
  Files          1217     1217              
  Lines        171874   171901      +27     
  Branches     139631   139657      +26     
============================================
+ Hits         144449   144490      +41     
+ Misses        23496    23458      -38     
- Partials       3929     3953      +24     
Components Coverage Δ
Rust Core 84.93% <76.10%> (+<0.01%) ⬆️
Java SDK 66.67% <ø> (ø)
C# SDK 75.05% <ø> (+0.07%) ⬆️
Python SDK 90.06% <89.58%> (-0.04%) ⬇️
PHP SDK 85.65% <ø> (ø)
Node SDK 95.90% <ø> (ø)
Go SDK 68.29% <ø> (ø)
Files with missing lines Coverage Δ
foreign/python/src/client.rs 99.85% <100.00%> (+<0.01%) ⬆️
foreign/python/src/consumer.rs 81.34% <87.50%> (-0.90%) ⬇️
core/sdk/src/clients/consumer.rs 72.52% <76.10%> (-0.36%) ⬇️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ethanlin01x
ethanlin01x force-pushed the fix/python-consumer-metadata-deadlock branch from 8d0d07d to b73cf67 Compare August 15, 2026 09:28
@ethanlin01x
ethanlin01x marked this pull request as ready for review August 15, 2026 09:41
@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Aug 15, 2026
@ethanlin01x

Copy link
Copy Markdown
Contributor Author

/request-review @hubcio

@ethanlin01x

Copy link
Copy Markdown
Contributor Author

/ready

@github-actions
github-actions Bot requested a review from hubcio August 15, 2026 10:39
@ethanlin01x
ethanlin01x force-pushed the fix/python-consumer-metadata-deadlock branch from 7b7a74e to 5763540 Compare August 20, 2026 13:19
The synchronous getters on IggyConsumer took the consumer mutex with
blocking_lock() while holding the GIL, and consume_messages holds that mutex
for the whole consumption run. Reading an attribute during consumption hung the
interpreter; reading one from a callback panicked inside the Tokio runtime.

None of those getters need exclusive access. The name, stream and topic are
fixed at construction, and the partition id and offsets live behind Arcs that
IggyConsumerState now exposes as a cloneable view. IggyConsumer owns that state
and delegates to it, so the Python wrapper reads metadata without the lock.

Found while reviewing apache#3776.
@ethanlin01x
ethanlin01x force-pushed the fix/python-consumer-metadata-deadlock branch from 5763540 to e9c641e Compare August 20, 2026 13:40
slbotbm
slbotbm previously approved these changes Aug 23, 2026

@hubcio hubcio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fix is good, some polishing is needed tho.

three that don't fit on a diff line:

  • foreign/python/src/consumer.rs:410-411: the lock_owned() guard is shadowed, not dropped, so it spans the whole consume() including the callback await. harmless today, but self.task_locals.lock().await.clone() drops it.
  • foreign/python/src/consumer.rs:202: tokio::join! waits on the shutdown task even when consume already exited, and that task waits forever on Event.wait(). the new test is only safe because it sets shutdown_event in finally.
  • nothing regenerates and diffs apache_iggy.pyi in ci - php has that gate, python doesn't. no impact here, separate ticket.

Comment thread core/sdk/src/clients/consumer.rs Outdated
Comment thread core/sdk/src/clients/consumer.rs
Comment thread core/sdk/src/clients/consumer.rs
Comment thread foreign/python/src/consumer.rs Outdated
Comment thread foreign/python/src/consumer.rs Outdated
Comment thread foreign/python/src/consumer.rs Outdated
Comment thread foreign/python/src/consumer.rs Outdated
Comment thread foreign/python/src/consumer.rs Outdated
Comment thread foreign/python/tests/test_consumer_group.py Outdated
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Aug 25, 2026
@slbotbm

slbotbm commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@hubcio do we want to check the pyi file in ci? i ask because this check will require a complete recompilation of the sdk, and our ci runs are very heavy as it is.

maybe we could instead redesign the ci for ffi sdks instead?

@hubcio

hubcio commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@slbotbm like we discussed on discord, please propose something via issues or discussions.

The flush loops held a DashMap shard read guard across the store await while
the poll future can insert a new partition into the same map.
A consumption run holds the consumer mutex while it awaits the callback, so
awaiting store_offset or delete_offset from inside one deadlocked. Under
AutoCommit.Disabled() that left no manual commit path at all.

Both take &self in the Rust SDK, so IggyConsumerState now owns them and the
Python wrapper reads through it instead of taking the lock.
tokio::join! waited on a task that parks on Event.wait() forever once consuming
has returned on its own, so it is now aborted instead. Signalling a receiver
that is already gone is no longer an error, or it would mask the result of the
run it was trying to stop.

The task locals guard was shadowed rather than dropped, so it spanned the whole
consume including the callback await.
IggyConsumerState is public through the prelude and had no Debug. `None` from
the offset getters means the partition is untracked, not that nothing was
consumed, and stream() / topic() give back an identifier rather than a name.
faulthandler_timeout and faulthandler_exit_on_timeout give the whole suite what
one test body was doing by hand.
# Conflicts:
#	core/sdk/src/clients/consumer.rs
#	foreign/python/src/consumer.rs
Rust 1.98's clippy flags the enum for a large size difference between
variants under -D warnings.
@ethanlin01x
ethanlin01x requested review from hubcio and slbotbm August 25, 2026 17:57
@ethanlin01x

ethanlin01x commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

@hubcio Thanks for the review! Also fixed a clippy error while at it.
For the apache_iggy.pyi CI gate, I'll dig into it and open a separate issue for discussion.

@ethanlin01x

Copy link
Copy Markdown
Contributor Author

/ready

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Aug 26, 2026
@hubcio
hubcio merged commit d7c2b61 into apache:master Aug 26, 2026
98 checks passed
@github-actions github-actions Bot removed the S-waiting-on-review PR is waiting on a reviewer label Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants