Skip to content

7842: Use select! instead of joins in connect_(tcp|udp) examples#48

Open
martin-augment wants to merge 2 commits intomasterfrom
pr-7842-2026-01-08-04-28-15
Open

7842: Use select! instead of joins in connect_(tcp|udp) examples#48
martin-augment wants to merge 2 commits intomasterfrom
pr-7842-2026-01-08-04-28-15

Conversation

@martin-augment
Copy link
Owner

7842: To review by AI

@claude
Copy link

claude bot commented Jan 8, 2026

Pull Request Review

Critical Issue: Incorrect Semantic Change

This PR introduces a critical bug by replacing join/try_join! with select!. These macros have fundamentally different behaviors:

  • join/try_join!: Waits for all futures to complete
  • select!: Completes when the first future completes and cancels the others

Impact on examples/connect-tcp.rs (lines 67-72)

Before:

match future::join(sink.send_all(&mut stdin), stdout.send_all(&mut stream)).await {
    (Err(e), _) | (_, Err(e)) => Err(e.into()),
    _ => Ok(()),
}

After:

tokio::select! {
    r = sink.send_all(&mut stdin) => r?,
    r = stdout.send_all(&mut stream) => r?,
}
Ok(())

Problem: The new code will exit as soon as either:

  • All stdin data is sent to the server, OR
  • All server data is sent to stdout

This means if stdin closes first, the program will exit without reading remaining data from the server. Conversely, if the server closes the connection first, any remaining stdin data won't be sent.

Impact on examples/connect-udp.rs (lines 62-65)

Before:

tokio::try_join!(send(stdin, &socket), recv(stdout, &socket))?;

After:

tokio::select! {
    r = send(stdin, &socket) => r?,
    r = recv(stdout, &socket) => r?,
}

Problem: Same issue - the program will exit when either send or recv completes, instead of running both concurrently until both are done.

Recommendation

This PR should not be merged as it fundamentally breaks the bidirectional communication behavior of these examples.

If the goal is to demonstrate select!, these examples are not appropriate candidates because they require both operations to run to completion. select! is better suited for scenarios where you want to race multiple operations or handle whichever completes first.

To fix this, you would need to revert to join/try_join! or implement more complex logic that continues running the remaining future after one completes.

Additional Notes

  • Code Quality: The code is well-formatted and follows Rust conventions
  • Security: No security concerns introduced (beyond the functional bug)
  • Performance: Not applicable due to the semantic incorrectness
  • Test Coverage: These are examples without tests, but the bug would be evident in runtime behavior

Verdict: ❌ Request changes - this PR introduces incorrect behavior

@augmentcode
Copy link

augmentcode bot commented Jan 8, 2026

🤖 Augment PR Summary

Summary: Switches the TCP/UDP connect examples from join!/try_join! to tokio::select! when forwarding stdin/stdout.

Why: Lets the example exit as soon as either the send or receive path completes (e.g., peer closes or stdin hits EOF), instead of potentially hanging while waiting for both sides to finish.

🤖 Was this summary useful? React with 👍 or 👎

Copy link

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 1 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

match future::join(sink.send_all(&mut stdin), stdout.send_all(&mut stream)).await {
(Err(e), _) | (_, Err(e)) => Err(e.into()),
_ => Ok(()),
tokio::select! {
Copy link

Choose a reason for hiding this comment

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

tokio::select! will end connect() as soon as either direction finishes, which means an stdin EOF will now stop draining any remaining socket output (and the non-selected branch’s error/result is dropped). If the example is intended to behave like a full duplex “pipe” until the peer closes, it might be worth double-checking this termination semantics (also applies to the UDP change).

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Copy link
Owner Author

Choose a reason for hiding this comment

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

value:incorrect-but-reasonable; category:bug; feedback:The Augment AI reviewer is not correct! The connect-tcp/udp examples are just demos of how to connect to a server and send/receive data to/from it. The proposed changes suggest to exit the client application (e.g. connect-tcp) if either of its stdin or stdout handles are closed. After closing the stdin of the connect-tcp app it won't be able to send any more data and it won't be usable.

_ => Ok(()),
tokio::select! {
r = sink.send_all(&mut stdin) => r?,
r = stdout.send_all(&mut stream) => r?,
Copy link

Choose a reason for hiding this comment

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

Using select! causes premature termination and data loss

Medium Severity

Replacing join/try_join! with select! fundamentally changes the semantics. The original code waited for both stdin→socket and socket→stdout operations to complete. The new code exits as soon as either completes, cancelling the other. When stdin reaches EOF, the program now exits immediately without waiting for pending server responses, potentially losing data. The examples are documented to "print all data received on stdout," which this change breaks.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Owner Author

Choose a reason for hiding this comment

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

value:incorrect-but-reasonable; category:bug; feedback:The Bugbot AI reviewer is not correct! The connect-tcp/udp examples are just demos of how to connect to a server and send/receive data to/from it. The proposed changes suggest to exit the client application (e.g. connect-tcp) if either of its stdin or stdout handles are closed. After closing the stdin of the connect-tcp app it won't be able to send any more data and it won't be usable.

@martin-augment
Copy link
Owner Author

Critical Issue: Incorrect Semantic Change

This PR introduces a critical bug by replacing join/try_join! with select!. These macros have fundamentally different behaviors:

  • join/try_join!: Waits for all futures to complete
  • select!: Completes when the first future completes and cancels the others

Impact on examples/connect-tcp.rs (lines 67-72)

Before:

match future::join(sink.send_all(&mut stdin), stdout.send_all(&mut stream)).await {
    (Err(e), _) | (_, Err(e)) => Err(e.into()),
    _ => Ok(()),
}

After:

tokio::select! {
    r = sink.send_all(&mut stdin) => r?,
    r = stdout.send_all(&mut stream) => r?,
}
Ok(())

Problem: The new code will exit as soon as either:

  • All stdin data is sent to the server, OR
  • All server data is sent to stdout

This means if stdin closes first, the program will exit without reading remaining data from the server. Conversely, if the server closes the connection first, any remaining stdin data won't be sent.

value:incorrect-but-reasonable; category:bug; feedback:The Claude AI reviewer is not correct! The connect-tcp/udp examples are just demos of how to connect to a server and send/receive data to/from it. The proposed changes suggest to exit the client application (e.g. connect-tcp) if either of its stdin or stdout handles are closed. After closing the stdin of the connect-tcp app it won't be able to send any more data and it won't be usable.

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