Skip to content

chore: Remove obsolete panic comments - #346

Open
uklotzde wants to merge 1 commit into
HMIProject:mainfrom
uklotzde:remove-obsolete-panic-comments
Open

chore: Remove obsolete panic comments#346
uklotzde wants to merge 1 commit into
HMIProject:mainfrom
uklotzde:remove-obsolete-panic-comments

Conversation

@uklotzde

Copy link
Copy Markdown
Collaborator

No description provided.

@uklotzde
uklotzde requested a review from sgoll July 15, 2026 10:19
@uklotzde uklotzde added this to the v0.12.1 milestone Jul 15, 2026
Comment on lines -116 to -118
// PANIC: When `callback` is called (which owns `tx`), we always call `tx.send()`. So the sender
// is only dropped after placing a value into the channel and `rx.await` always finds this value
// there.

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.

I think I would preserve the comments as such, explaining why we can unwrap into an Err here and why that typically shouldn't be possible to happen.

Suggested change
// PANIC: When `callback` is called (which owns `tx`), we always call `tx.send()`. So the sender
// is only dropped after placing a value into the channel and `rx.await` always finds this value
// there.
// This error _should_ never happen: When `callback` is called (which owns `tx`), we always call
// `tx.send()`. So the sender is only dropped after placing a value into the channel. `rx.await`
// should therefore always be able to find this value there.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It could also happen when the runtime is shut down. Tasks are aborted in no particular order and the sender task might be cancelled before the receiver task. The comment doesn't account for this.

https://www.mattkeeter.com/blog/2024-08-01-panic/

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.

I think that blog post doesn't apply here: in our case, the sending happens inside a C callback, so this happens outside the async runtime's control and regardless of it, i.e. the runtime cannot cancel this thread. In fact, the ownership of the sender has been moved inside our callback wrapper, so it is only dropped if and when that callback fires (see implementation of CallbackOnce; this is meant by "callback owns tx").

Therefore, I think the comment still holds: shutting down the runtime does not drop the sender, so if we end up here, we always find the sender alive and we never run into the unwrap() branch.

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.

2 participants