Skip to content

fix direct connector only select one listener#875

Merged
KKRainbow merged 1 commit intomainfrom
v6_hole_punch
May 25, 2025
Merged

fix direct connector only select one listener#875
KKRainbow merged 1 commit intomainfrom
v6_hole_punch

Conversation

@KKRainbow
Copy link
Copy Markdown
Member

No description provided.

@KKRainbow KKRainbow requested a review from Copilot May 24, 2025 16:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where only one listener was being selected for a direct connector and introduces refactored connection task handling. Key changes include:

  • Adding debug logging for connection attempts.
  • Splitting the original connection function into spawn_direct_connect_task and an updated do_try_direct_connect_internal.
  • Sorting available listeners to prioritize the default protocol and UDP.

Comment on lines 285 to 297
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

Consider handling the socket address parsing error more explicitly. Returning a Result instead of silently logging and returning might improve error propagation and overall error handling clarity.

Suggested change
) {
let Ok(mut addrs) = listener.socket_addrs(|| None) else {
tracing::error!(?listener, "failed to parse socket address from listener");
return;
};
) -> Result<(), Error> {
let mut addrs = listener
.socket_addrs(|| None)
.map_err(|e| {
tracing::error!(?listener, ?e, "failed to parse socket address from listener");
Error::InvalidSocketAddress
})?;

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

Add a documentation comment for spawn_direct_connect_task to clarify its intended role in spawning tasks and handling connection attempts, which will aid future maintenance.

Copilot uses AI. Check for mistakes.
@KKRainbow KKRainbow requested a review from Copilot May 25, 2025 05:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where the direct connector only selects one listener by refining the connection retry logic and updating the listener selection flow. Key changes include extending the backoff intervals, refactoring task spawning and listener selection into dedicated functions with improved logging, and removing an unused dependency from Cargo.toml.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
easytier/src/connector/direct.rs Adjusts connection retry logic, refactors task spawning with improved logging and sorting.
easytier/Cargo.toml Removes the bounded_join_set dependency to reflect the new task join set usage.
Comments suppressed due to low confidence (1)

easytier/Cargo.toml:215

  • Ensure that your code now imports and uses the intended JoinSet type (e.g., from tokio) to avoid any unresolved import issues after removing the bounded_join_set dependency.
-bounded_join_set = "0.3.0"

tracing::error!(?e, "try direct connect to peer task join failed");
}

tracing::debug!("try direct connect to peer with listener: {}", listener);
Copy link

Copilot AI May 25, 2025

Choose a reason for hiding this comment

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

For consistency with other structured logging calls in the file, consider using the structured logging syntax (e.g., tracing::debug!(?listener, ...)) instead of relying on string formatting.

Suggested change
tracing::debug!("try direct connect to peer with listener: {}", listener);
tracing::debug!(?listener, "try direct connect to peer with listener");

Copilot uses AI. Check for mistakes.
@KKRainbow KKRainbow merged commit b0fd379 into main May 25, 2025
32 checks passed
@KKRainbow KKRainbow deleted the v6_hole_punch branch May 25, 2025 05:56
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