Skip to content

fix: MeasureManyStream now properly terminates #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
paolobarbolini merged 2 commits into from
Jan 23, 2026
Merged

fix: MeasureManyStream now properly terminates #4

paolobarbolini merged 2 commits into from
Jan 23, 2026

Conversation

@ThinkerDreamer
Copy link
Member

@ThinkerDreamer ThinkerDreamer commented Jan 22, 2026 •
edited
Loading

Summary

  • MeasureManyStream now returns None when all pings complete, allowing while let loops to exit
  • Added v4_done/v6_done tracking to DualstackMeasureManyStream for proper completion

Problem

The stream never returned None to signal completion. After all ping responses were received, poll_next_unpin would return Poll::Pending forever, causing while let Some(...) = stream.next().await loops to hang indefinitely.

Solution

  • BREAKING CHANGE! Changed poll_next_unpin return type from Poll<(V, Duration)> to Poll<Option<(V, Duration)>>
  • Added completion check: when send_queue is empty AND in_flight is empty, return Poll::Ready(None)
  • Updated Stream impl to pass through the Option directly
  • Added completion state tracking to DualstackMeasureManyStream

Test plan

  • New regression tests verify stream terminates after single/multiple pings
  • Test verifies empty address list terminates immediately
  • Tests run on both current_thread and multi_thread runtimes
  • All existing tests pass

Closes #3

@ThinkerDreamer
Copy link
Member Author

ThinkerDreamer commented Jan 22, 2026

This PR also includes the changes from #2 currently while we wait for that to be merged.

ThinkerDreamer
ThinkerDreamer commented Jan 22, 2026
View reviewed changes
dodomorandi
dodomorandi suggested changes Jan 23, 2026
View reviewed changes
Copy link

@dodomorandi dodomorandi left a comment

Choose a reason for hiding this comment

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

It needs a rebase

@ThinkerDreamer
fix!: MeasureManyStream now properly terminates
5514d48 
The stream now returns None when all ping requests have been sent and
all responses received. Previously, poll_next_unpin would return
Poll::Pending forever after processing all results, causing
while let loops to hang indefinitely.

Changes:
- poll_next_unpin now returns Poll<Option<...>> instead of Poll<...>
- Added completion check: when send_queue is empty AND in_flight is
  empty, return Poll::Ready(None)
- Updated Stream impl to pass through the Option directly
- Added v4_done/v6_done flags to DualstackMeasureManyStream to
  track completion state of both underlying streams
- Removed unused task::ready imports

Closes M4SS-Code#3
@ThinkerDreamer
test: add regression tests for stream termination
87895f3 
Tests verify that MeasureManyStream properly returns None when:
- Single ping completes
- Multiple pings to different addresses complete
- Empty address list is provided

Tests run on both current_thread and multi_thread runtimes.
@paolobarbolini paolobarbolini merged commit 4fbc22f into M4SS-Code:main Jan 23, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paolobarbolini paolobarbolini Awaiting requested review from paolobarbolini

1 more reviewer

@dodomorandi dodomorandi dodomorandi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

MeasureManyStream never terminates (returns None)

3 participants

@ThinkerDreamer @dodomorandi @paolobarbolini