Repository navigation
Conversation
64050f2 to
e723c15
Compare
Marenz
left a comment
There was a problem hiding this comment.
🤖 LGTM. This fixes real bugs, and the tests cover the hard cases (GC in another thread, closed loops, double detach). The release notes are accurate. ci_checks_max and pytest_min pass locally (152 tests).
Nits:
- The PR description says "The channel still closes on the next
send()without receivers, as in 1.12". The code does the opposite:send()raisesNoReceiversErrorand keeps the channel open (_broadcast.py:499), as commit 8278c97 and the release notes say. Please fix the description and add this to the "Breaking" list (before, that send closed the channel and raisedSenderErrorwith aChannelClosedErrorcause). BroadcastReceiver.close()wakes receivers directly (_broadcast.py:756), while the sender finalizer usescall_soon_threadsafe(). With asyncio debug off, callingclose()from another thread leaves a waiting task stuck without any error; the thread test only covers debug mode. A docstring note is probably enough, or route it through_wake_receivers_soon()._sender_count -= 1(_broadcast.py:182) runs in the finalizer, which can be any thread, and is not atomic. Worth a comment that only the wake-up is thread-safe._Broadcastis private, so there is no public type left to checkChannelClosedError.channelagainst. Maybe say so in the release notes.
e723c15 to
6865c28
Compare
llucax
left a comment
There was a problem hiding this comment.
My main concern here is I'm not convinced about the messing around with the GC, the rest is mostly minor. I will add another review/comment about the breaking changes and how we should ship this (v1 or v2).
I also did a AI review:
🤖 rAIview (AI review here!)
The lifecycle fixes and expanded documentation address several real problems. I found one remaining garbage-collection regression: the endpoint finalizers retain channels strongly, which can keep otherwise unreachable channels and cached payloads alive indefinitely. Both finalizer callbacks should refer to the channel weakly.
|
|
||
| self._channel._sender_count += 1 | ||
| # pylint: disable=protected-access | ||
| self._detach = weakref.finalize(self, channel._remove_sender) |
There was a problem hiding this comment.
🟡⬇️ Avoid retaining channels strongly in endpoint finalizers
weakref.finalize(self, channel._remove_sender) stores a bound method, so the finalizer registry keeps channel alive. The receiver finalizer does the same with channel._remove_receiver. Although the channel stores its receivers weakly, it stores _latest strongly, even with the default resend_latest=False. A message can therefore create an indirect strong reference back to an endpoint, for example when two channels exchange reply senders, or when a message contains an object that owns an endpoint.
Once every application reference is dropped, the graph is still reachable through finalize registry → bound callback → channel → _latest → endpoint. This is not just a collectable reference cycle: the global registry prevents collection, so the finalizer that would release the graph never runs. Python's weakref.finalize documentation explicitly warns against callback references to the finalized object, including indirect references.
I reproduced this with two BroadcastChannel[object] instances that send each other's senders, drain both receiver buffers, and then drop all four endpoints. After gc.collect(), both senders remain alive on this PR, while all four endpoints are collected on the base branch. The receiver callback independently causes the same problem for a legacy Broadcast whose cached message references its receiver, even after its sender is explicitly closed.
Impact
Applications that repeatedly create and discard channels with endpoint-containing payloads can accumulate channels, finalizers, cached messages, and everything those messages reference until process exit or explicit cleanup. Draining the receiver buffers does not avoid the leak.
Related locations
- Receiver finalizer: this callback also needs to avoid retaining the channel strongly.
- Cached message assignment: every successful send stores the payload in
_latest, regardless ofresend_latest.
Suggested fix (⬇️ effort)
Use a callback that receives only a weakref.ref(channel), dereferences it once, and calls the removal method only if the channel is still alive. Apply this to both endpoint finalizers, without capturing a bound channel method in the callback or its arguments. Endpoints already keep their channel alive while application references to them exist. A runtime probe using weak channel callbacks collected both reproduced cycles and still woke a live receiver when its last sender was collected.
Suggested tests
Add collection tests for both finalizers, including a cross-channel cycle with drained buffers and a receiver cycle after explicit sender closure.
Suggested new tests
import gc
import weakref
from frequenz.channels import Broadcast, BroadcastChannel
async def test_unreachable_cross_channel_cycle_is_collected() -> None:
sender1, receiver1 = BroadcastChannel[object](name="requests")
sender2, receiver2 = BroadcastChannel[object](name="replies")
refs = [weakref.ref(x) for x in (sender1, receiver1, sender2, receiver2)]
await sender1.send(sender2)
await sender2.send(sender1)
await receiver1.receive()
await receiver2.receive()
del sender1, receiver1, sender2, receiver2
gc.collect()
assert all(ref() is None for ref in refs)
async def test_unreachable_receiver_cycle_is_collected() -> None:
channel = Broadcast[object](name="receiver-cycle")
sender = channel.new_sender()
receiver = channel.new_receiver()
ref = weakref.ref(receiver)
await sender.send(receiver)
await receiver.receive()
await sender.aclose()
del channel, sender, receiver
gc.collect()
assert ref() is None🟡 Medium Severity: Probably best to fix before merging; skipping needs a good reason.
⬇️ Low Effort: A small callback change and focused regression tests.
There was a problem hiding this comment.
Fixed by removing both finalizers, so nothing global holds the channel any more.
| * [Anycast](anycast.md) | ||
| * [Broadcast](broadcast.md) | ||
| * [Oneshot](oneshot.md) | ||
| * [Anycast](anycast.md) (deprecated) |
There was a problem hiding this comment.
Nitpick: This was sorted alphabetically, I would keep the order. We could also just remove it from here, do don't need to promote deprecated stuff...
| def _remove_sender(self, *, from_gc: bool = True) -> None: | ||
| """Detach a closed or garbage-collected sender from this channel. | ||
|
|
||
| The garbage collector can run this in any thread, and in the middle of | ||
| other code. So when it does, this only updates the channel's state, and | ||
| the receivers are woken up later, on the event loop they wait on. | ||
|
|
||
| Args: | ||
| from_gc: Whether the garbage collector runs this, through the sender's | ||
| finalizer. | ||
| """ | ||
| self._sender_count -= 1 | ||
| if self._auto_close_enabled and self._sender_count == 0: | ||
| if from_gc: | ||
| self._mark_closed() | ||
| self._wake_receivers_soon() | ||
| else: | ||
| self._close() |
There was a problem hiding this comment.
I'm not sure about this (and the "Auto-close broadcast channels when the last sender is dropped" commit in general). Is this supposed to only fix when someone forgets to call aclose() on the receiver, (a bug basically) or is this handling a valid use case?
Senders that were garbage-collected without
aclose()...
I can't see any cases where a sender could be garbage-collected without aclose() that is not a user forgetting to call aclose() (a bug).
The garbage collector can detach a sender in any thread, or in the
middle ofEvent.wait().
Why? How can the garbage collector detach anything?
If it is to catch a case where users forgot the aclose() I wouldn't go to the trouble of trying to clean stuff for them. All the weakref.finalize() business is usually very fragile, and I wouldn't be surprised if we introduce more subtle bugs with it, so it is basically extra complexity (storing the loop, calling call_soon_threadsafe(), using weakref.finalize(), having code running inside the GC, etc.) for nothing.
There was a problem hiding this comment.
Damn it, it is for handling the user bug of forgetting to close, but it would have been very useful for that, especially with all the agentic coding and it is also get super hard to find out which receiver or sender wasn't closed, and we'll give up too easily instead of trying to debug properly.
But I agree that the implementation is buggy and looks like it can't be perfect, so I've dropped it.
| def _remove_receiver(self, key: int) -> None: | ||
| """Detach a closed or garbage-collected receiver from this channel. | ||
|
|
||
| Args: | ||
| key: The key of the receiver in `self._receivers`. | ||
| """ | ||
| self._receivers.pop(key, None) |
There was a problem hiding this comment.
Same for commit "Detach garbage-collected broadcast receivers at once".
Garbage-collected receivers stayed in the channel until the next
send()swept them out. They now detach as soon as they are collected, so the channel doesn't keep dead receivers between sends.
Why do we need this? If an opened receiver is garbage collected, that's a user bug IMHO, they should close them before forgetting the last reference to them.
| @property | ||
| def has_senders(self) -> bool: | ||
| """Whether this receiver's channel is open and has any senders. | ||
|
|
||
| A [`BroadcastChannel`][frequenz.channels.BroadcastChannel] is closed as soon | ||
| as its last sender is closed or garbage-collected. So once this is `False` | ||
| on a `BroadcastChannel`, it stays `False`, and no new messages will arrive. | ||
| The messages already in this receiver's buffer can still be received. | ||
|
|
||
| This doesn't check whether this receiver itself is closed. | ||
| """ | ||
| # pylint: disable=protected-access | ||
| return not self._channel._closed and self._channel._sender_count > 0 |
There was a problem hiding this comment.
What about num_senders (and num_receivers) instead? You can still use it as a bool but can also get the amount.
There was a problem hiding this comment.
sender_count already exists, IIRC, that was the old way of identifying whether someone should wait on a receiver or not, don't remember.
Any way, num_senders would be weird when there's also sender_count.
Also, sender_count sends a value even if the receiver is closed, which might not be ideal, I guess we should actually deprecate that.
has_receivers etc gives enough info. I think exact counts is too much info, at least for the way we want to be using this field.
| # The docstrings carry long examples. | ||
| # pylint: disable=too-many-lines |
There was a problem hiding this comment.
Nitpick: We could consider splitting into more files, like _broadcast_sender.py and _broadcast_receiver.py or such.
There was a problem hiding this comment.
Fixed by trimming the docstring of the deprecated Broadcast, asking people to use BroadcastChannel instead.
| !!! Warning "Deprecated" | ||
|
|
||
| Please use [`BroadcastChannel`][frequenz.channels.BroadcastChannel] instead. | ||
|
|
There was a problem hiding this comment.
I would remove this, the next repo-config will add the admonition automatically.
There was a problem hiding this comment.
If we remove this file from the index, we should probably remove the file too.
| * [`BroadcastChannel`][frequenz.channels.BroadcastChannel]: A channel to broadcast | ||
| messages from multiple senders to multiple receivers. Each message sent through any | ||
| of the senders is received by all of the receivers. The channel closes itself when | ||
| all its senders are closed or garbage-collected. A `send()` while it has no |
There was a problem hiding this comment.
If we stop handling garbage collected stuff, we should remove these (and all other gc mentions) too.
| except SenderError as error: | ||
| if isinstance(error, NoReceiversError): | ||
| _logger.warning( | ||
| "Pipe from [%s] to [%s] stopped: the sender has no receivers", | ||
| self._receiver, | ||
| self._sender, | ||
| ) | ||
| else: | ||
| _logger.exception( | ||
| "Pipe from [%s] to [%s] stopped: sending failed", | ||
| self._receiver, | ||
| self._sender, | ||
| ) |
There was a problem hiding this comment.
What? Why not:
| except SenderError as error: | |
| if isinstance(error, NoReceiversError): | |
| _logger.warning( | |
| "Pipe from [%s] to [%s] stopped: the sender has no receivers", | |
| self._receiver, | |
| self._sender, | |
| ) | |
| else: | |
| _logger.exception( | |
| "Pipe from [%s] to [%s] stopped: sending failed", | |
| self._receiver, | |
| self._sender, | |
| ) | |
| except NoReceiversError: | |
| _logger.warning( | |
| "Pipe from [%s] to [%s] stopped: the sender has no receivers", | |
| self._receiver, | |
| self._sender, | |
| ) | |
| except SenderError: | |
| _logger.exception( | |
| "Pipe from [%s] to [%s] stopped: sending failed", | |
| self._receiver, | |
| self._sender, | |
| ) |
llucax
left a comment
There was a problem hiding this comment.
I checked several internal consumers to understand the practical compatibility impact. Most still use the legacy, non-auto-closing Broadcast, so these changes appear to affect us less than the breaking-change list initially suggests. Only a couple of the inspected consumers use Pipe, and their normal ownership and teardown patterns don’t look immediately broken by this change. That is reassuring for our current usage, but it doesn’t establish compatibility for other users.
I’d suggest the following release plan:
- Release the backward-compatible fixes and additive APIs in 1.x, while preserving existing defaults and public contracts.
- Make the new behavior for sending without receivers opt-in in 1.x. The existing behavior should remain the default; users should explicitly choose to raise
NoReceiversErrorwithout closing the channel. - Optionally, make
Pipe’s new endpoint-closing behavior opt-in in 1.x too. Closing caller-supplied endpoints changes ownership expectations, even if the current internal call sites appear compatible. However, given thatPipeis experimental, I’d also be comfortable making an explicit, documented exception and shipping this breaking change in 1.x.
For each compatibility option we introduce, I’d use this transition:
| Release | Default and migration |
|---|---|
| 1.x | Preserve the old behavior; allow opting into the new behavior. |
| 2.x | Make the new behavior the default, but retain the compatibility option. Emit a DeprecationWarning when the old behavior is explicitly selected. |
| 3.x | Remove the old behavior and the compatibility option. |
Keeping the options in 2.x would let applications adopt the new behavior while still on 1.x and carry that configuration across the major-version upgrade. Once we agree on the approach, we can track their eventual removal in a separate 3.x breaking-changes issue.
If implementing and maintaining this compatibility would be too much work, I’d instead create a v2.x.x branch and target this PR there. We already have some accumulated deprecations, so we could also take the opportunity to remove deprecated APIs and do some cleanup for 2.0.0. That would make it a useful major release rather than introducing compatibility machinery just to keep these changes in 1.x.
Separately, if we already rely on Pipe in real applications, I think it’s probably time to promote it out of experimental. That should include clear documentation of endpoint ownership, normal completion, send failures, and cancellation, plus a migration path for the existing import. It could be a separate focused PR, but it seems worth doing alongside this lifecycle work.
6865c28 to
8b21c5b
Compare
`BroadcastReceiver.close()` is synchronous and couldn't notify the channel's condition variable, so a task waiting in `ready()` hung until the next message arrived or the channel was closed. Receivers now wait on an event that can be set from synchronous code. Waiters left on a closed event loop are skipped, so they don't make `close()` fail. For the same reason, `send()` and `aclose()` no longer raise `RuntimeError: Event loop is closed` because of them. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
A closed receiver with messages left in its buffer kept returning True from `ready()`, while `consume()` raised `ReceiverStoppedError`. So `select()` yielded stopped selections for it forever, and a `filter()` on it made `select()` fail. Closing a receiver now drops its buffered messages, so it is never ready. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Like tokio's broadcast channel, a send to an auto-closing broadcast channel without receivers still fails, but now leaves the channel open, so new receivers can still be subscribed. `send()` raises a `NoReceiversError` and drops the message, but with `resend_latest=True` it still keeps the message, so new receivers get the real latest one. Such a channel now closes itself only when it has no open senders. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Unlike `send()` and `subscribe()`, `clone()` worked on a closed sender or channel. On a closed channel, it handed out senders that could never send. It now raises like `send()` and `subscribe()`, through a check that the three methods share. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
`limit` and `warn_on_overflow` were documented as applying to the channel's receivers, but only the initial receiver got them; `subscribe()` always fell back to 50 and `True`. They are now the defaults for `subscribe()`. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Lets producers stop when nobody is listening anymore, and consumers tell whether more messages can still arrive, without having to send or wait on the channel. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
`BroadcastChannel` was built on the deprecated `Broadcast` class, so creating one warned users to "use BroadcastChannel instead". The class is now private, and `Broadcast` is a deprecated alias of it, made with `deprecated_aliases()` from `frequenz-core`. Reaching the alias warns, creating a `BroadcastChannel` doesn't. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
`Broadcast` is deprecated, but its docstring still repeated the channel description and examples of `BroadcastChannel`. It now only says how to use the class. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
About auto-closing, the docs only said that closing all senders closes all receivers, and what a send without receivers does. They now also cover what each side sees, that an unclosed sender keeps the channel open, and `has_receivers`/`has_senders`. The multi-sender example is fixed too: it claimed to use a small buffer to make the senders block, it caught a `ReceiverStoppedError` that `async for` never raises, and its receivers slept once after the loop instead of after each message. The first example now stops when its sender is closed. The `BroadcastSender` and `BroadcastReceiver` docs now point to `BroadcastChannel`, and the `resend_latest` docs point to `subscribe()`. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The `aclose()` docs said that receiving after the buffer is drained returns `None`; they now say a `ReceiverStoppedError` is raised. The receiver guide, `Receiver.close()` and the `map()` and `filter()` receivers said every closed receiver drains its buffer first; they now say this depends on the receiver. `BroadcastReceiver.close()` doesn't reliably wake up tasks when called from another thread, so its docs now say where to call it. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The user guide, package overview and README still presented the deprecated `Broadcast` and `Anycast` classes. They now show `BroadcastChannel`, the overview marks `Anycast` as deprecated, and the user guide drops its `Anycast` page. A new page covers `OneshotChannel`. The README showcase now closes its data senders, and its receiver stops when the data channels close. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The examples used the deprecated `Anycast` and `Broadcast` classes. Also fix some examples with missing imports, and the latest value cache examples, which checked the cache before its task could run. The `Anycast` docs now point to `BroadcastChannel`. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
A `Pipe` whose receiver stopped, or whose send failed with a `SenderError`, ended but kept its sender open, so the receivers of the sender's channel could wait forever. Since a broadcast channel now stays open without receivers, receivers subscribed after a failed send waited forever too. And the pipe's receiver stayed attached to its source channel, so `has_receivers` stayed `True` there. Close the sender in both cases, and the receiver too when the send failed. `stop()` still leaves both open. After a send fails with a `SenderError`, the pipe now logs the error itself: a `NoReceiversError` as a warning, and any other error with its traceback. Its task then ends normally, so asyncio logs nothing. If closing one of the pipe's ends fails, the task ends with that error. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
A failed send stops a `Pipe`, which is wrong when no receivers is a normal state of the sender's channel. With `close_pipe_on_send_error=False`, the pipe drops the message and keeps sending the messages that follow. A stopped receiver still closes the sender and stops the pipe. With that option, a missing receiver is expected, so it is logged at debug level, not as a warning for each message. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
8b21c5b to
42053e0
Compare
|
Also added |
BroadcastChannel(new in 1.12) closed itself on the first send without receivers, and producers had no way to tell whether anyone was listening. This branch keeps the channel open in that case, like tokio's broadcast channel, fixes several gaps in closing, and addsBroadcastSender.has_receiversandBroadcastReceiver.has_senders.Changes
Eventthat is replaced on each wake-up, instead of anasyncio.Condition. Waiters left on a closed event loop no longer makeclose(),send()oraclose()raise. Please look closely at_wake_receivers()andBroadcastReceiver.ready().select()no longer loops on it.send()without receivers raises the newNoReceiversError(aSenderError) and drops the message, but leaves the channel open, so new receivers can still be subscribed. Withresend_latest=True, the message is still kept, so a new receiver gets the real latest value. Only closing the senders closes the channel.contextlib.aclosing().clone()refuses a closed sender or channel.subscribe()uses thelimitandwarn_on_overflowthe channel was created with as its defaults.BroadcastChannelno longer emitsBroadcast's deprecation warning. The class is now private, andBroadcastis a deprecated alias of it, made withdeprecated_aliases()from the newfrequenz-coredependency. Its docstring is now short and points toBroadcastChannel.Pipecloses its sender when its receiver stops, and both ends when a send fails. Withclose_pipe_on_send_error=False, it drops the message after a failed send and keeps sending the messages that follow.stop()still leaves both open.BroadcastChannel. There is a new Oneshot page, the Anycast page is gone, and several false claims about closing are fixed.Breaking
send()without receivers.send()raisesNoReceiversErrorinstead, andsubscribe()keeps working.Pipenow closes its sender when its receiver stops, and both ends when a send fails with aSenderError.BroadcastSender.clone()raisesSenderClosedErrororSenderErroron a closed sender or channel.BroadcastSender.subscribe()takes itslimitandwarn_on_overflowdefaults from the channel.BroadcastReceiver.close()now wakes waiting tasks, so it must be called from the thread running the event loop.Broadcastdeprecation warning fires when the name is imported or accessed, not on creation, and type checkers no longer report it as deprecated.frequenz-core >= 1.6.0, < 2, and needstyping-extensions >= 4.13.0.