Skip to content

Conversation

@kixelated
Copy link
Collaborator

I think this is better than #525 and #524. The tokio documentation states that try_recv won't spuriously return Empty.

@claude
Copy link
Contributor

claude bot commented Aug 6, 2025

Claude finished @kixelated's task —— View job


Code Review Complete

Reviewing PR #526 for bugs and security issues:

  • Read and understand the changed files
  • Analyze the "now-or-never" fix implementation
  • Check for potential bugs and security vulnerabilities
  • Provide review feedback

Review Findings

👍 No critical bugs or security issues found. This is a clean fix that addresses a specific tokio concurrency issue.

Analysis

The Problem:

  • There was a tokio bug where only the first 127 broadcasts would be received instantly using futures::FutureExt::now_or_never()
  • The issue was in rs/moq-relay/src/web.rs:83 and rs/moq/src/session/publisher.rs:85 where announced.next().now_or_never() was used

The Solution:

  • Added a new try_next() method to OriginConsumer that uses mpsc::UnboundedReceiver::try_recv() instead of the problematic now_or_never() pattern
  • Replaced announced.next().now_or_never() calls with direct announced.try_next() calls
  • Removed unused futures dependency from moq-relay

Code Quality:

  • The new try_next() method is well-documented with clear semantics about when it returns None
  • Added is_closed() method for proper consumer state checking
  • Test coverage includes both the failing case (test_128 - should panic) and the fix (test_128_fix)
  • Proper error handling and boundary conditions

Security & Safety:

  • No unsafe code introduced
  • No potential for data races or concurrency issues
  • Method signatures prevent misuse (returns Option<OriginUpdate> consistently)
  • No sensitive information exposure

This fix is simpler and more reliable than the alternatives mentioned in #524 and #525, as it relies on tokio's documented guarantee that try_recv() won't spuriously return Empty.

@kixelated kixelated merged commit f7cf9a6 into main Aug 6, 2025
2 checks passed
@kixelated kixelated deleted the now-or-never-2 branch August 6, 2025 14:45
@moq-bot moq-bot bot mentioned this pull request Aug 6, 2025
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