WIP - ClientConnectionHandler

[jadamcrain]

Defines a new interface that provides fine-grained control over TCP/TLS client connection management.

[gemini-code-assist]

Hello @jadamcrain, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello! Gemini here, providing a summary of this pull request. This PR introduces a new ClientConnectionHandler trait designed to give users more fine-grained control over the TCP/TLS client connection lifecycle. The existing connection logic, previously handled by the Connector struct and the EndpointList, has been refactored. The ClientTask now delegates connection attempts and state management to an implementation of this new trait. A default SimpleConnectHandler is provided to maintain the previous behavior of cycling through endpoints with exponential backoff on failures and a fixed reconnect delay after disconnects. The master and outstation client spawning functions have been updated to accept this new handler, with new variants (spawn_master_tcp_client_3, spawn_master_tls_client_3, spawn_outstation_tcp_client_2, spawn_outstation_tls_client_2) added to allow users to provide their own custom handler implementations. Minor documentation fixes and a temporary lint disable are also included.

Highlights

• New Interface: Introduces the ClientConnectionHandler trait, allowing users to customize client connection behavior, including endpoint selection, retry logic, and state notifications.
• Refactoring: The internal Connector struct is removed, and its responsibilities are moved into the ClientTask and delegated to the new ClientConnectionHandler trait.
• Endpoint Management: The EndpointList struct is simplified, and the logic for iterating and resolving endpoints is now part of the connection handling process managed by the ClientConnectionHandler.
• New API Functions: Adds new spawn_*_client functions that accept a custom Box<dyn ClientConnectionHandler> for advanced use cases, while retaining the previous functions that now use a default handler.

Changelog

Click here to see the changelog

• Cargo.toml
  • Commented out the unused deny lint.
• dnp3/src/outstation/traits.rs
  • Minor documentation fixes in comments for OutstationApplication::handle_write_attribute and BroadcastAction::UnsupportedFunction.
  • Corrected a typo in the comment for ControlSupport::select.
• dnp3/src/tcp/client.rs
  • Removed ConnectStrategy, ConnectOptions, Connector, EndpointList imports.
  • Added ClientConnectionHandler, ConnectionInfo, EndpointInner, SocketAddr, Arc, TcpSocket imports.
  • Removed connector and reconnect_delay fields from ClientTask.
  • Added connect_handler and post_connection fields to ClientTask.
  • Updated ClientTask::new signature to accept connect_handler and post_connection.
  • Removed the internal connect method.
  • Added new internal connect, connect_to_endpoint, and connect_to_addr methods to handle connection attempts using the ClientConnectionHandler.
  • Updated run_phys to accept addr and hostname and use self.connect_handler.disconnected() for the reconnect delay.
• dnp3/src/tcp/connect.rs
  • Removed imports related to ExponentialBackOff, RetryStrategy, EndpointList, TcpSocket.
  • Made PostConnectionHandler::post_connect public (pub(crate)).
  • Fixed a typo in the ConnectOptions::set_local_endpoint documentation.
  • Removed the Connector struct and its implementation.
• dnp3/src/tcp/connector.rs
  • Added new file connector.rs.
  • Defined the Endpoint struct and EndpointInner enum.
  • Defined the ConnectionInfo struct.
  • Defined the ClientConnectionHandler trait with methods for connection lifecycle events (endpoint_span_name, disconnected, next, connecting, connect_failed, connected, resolution_failed).
  • Defined the SimpleConnectHandler struct and implemented ClientConnectionHandler for it, providing the default connection behavior.
• dnp3/src/tcp/endpoint_list.rs
  • Removed imports related to VecDeque, SocketAddr.
  • Added import for the new Endpoint struct.
  • Removed pending_endpoints and current_endpoint fields from EndpointList.
  • Added endpoints() method to convert internal strings to Endpoint structs.
  • Removed main_addr, reset, and next_address methods.
• dnp3/src/tcp/master/client.rs
  • Updated imports to include ClientConnectionHandler and SimpleConnectHandler.
  • Updated doc comment for spawn_master_tcp_client_2.
  • Added spawn_master_tcp_client_3 function that accepts a Box<dyn ClientConnectionHandler>.
  • Refactored spawn_master_tcp_client_2 to use SimpleConnectHandler::create and call spawn_master_tcp_client_3.
  • Updated the tracing::info_span name in the spawned future to use the name from the connect_handler.
  • Updated wire_master_client signature to accept connect_handler and removed old connection parameters.
  • Updated ClientTask::new call to pass the new connect_handler.
• dnp3/src/tcp/master/server.rs
  • Minor documentation fix in the comment for the ConnectionHandler trait.
• dnp3/src/tcp/mod.rs
  • Added pub use connector::*; to expose the new module.
• dnp3/src/tcp/outstation/client.rs
  • Updated imports to include ClientConnectionHandler and SimpleConnectHandler.
  • Added spawn_outstation_tcp_client_2 function that accepts a Box<dyn ClientConnectionHandler>.
  • Refactored spawn_outstation_tcp_client to use SimpleConnectHandler::create and call spawn_outstation_tcp_client_2.
  • Updated the tracing::info_span name in the spawned future to use the name from the connect_handler.
  • Updated ClientTask::new call to pass the new connect_handler.
• dnp3/src/tcp/tls/master.rs
  • Updated imports to include ClientConnectionHandler and SimpleConnectHandler.
  • Updated doc comment for spawn_master_tls_client_2.
  • Added spawn_master_tls_client_3 function that accepts a Box<dyn ClientConnectionHandler>.
  • Refactored spawn_master_tls_client_2 to use SimpleConnectHandler::create and call spawn_master_tls_client_3.
  • Updated the tracing::info_span name in the spawned future to use the name from the connect_handler.
  • Updated wire_master_client call to pass the new connect_handler.
• dnp3/src/tcp/tls/outstation.rs
  • Updated imports to include ClientConnectionHandler and SimpleConnectHandler.
  • Added spawn_outstation_tls_client_2 function that accepts a Box<dyn ClientConnectionHandler>.
  • Refactored spawn_outstation_tls_client to use SimpleConnectHandler::create and call spawn_outstation_tls_client_2.
  • Updated the tracing::info_span name in the spawned future to use the name from the connect_handler.
  • Updated ClientTask::new call to pass the new connect_handler.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

A trait defined,
Connection refined,
User control takes hold.
No longer bound,
New logic found,
A flexible story unfolds.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a significant and beneficial refactoring by defining the ClientConnectionHandler interface. This change greatly enhances the flexibility of TCP/TLS client connection management, allowing for fine-grained control over connection strategies. The existing behavior appears to be well-preserved through the SimpleConnectHandler implementation.

The code is generally clear and the refactoring seems robust. My main concern, detailed below, is related to a linter configuration change that should be clarified or reverted before merging, especially given the "WIP" status of this PR.

Summary of Findings

• Linter Configuration (unused rule): The unused lint rule in Cargo.toml has been commented out. This should ideally be re-enabled or its permanent removal justified before merging to maintain code quality.

Merge Readiness

Given the "WIP" status in the title and the commented-out unused lint in Cargo.toml, this pull request likely requires further changes or clarifications before it's ready for merging. Addressing the unused lint concern is important. Once these points are resolved and the author confirms the PR is no longer a work-in-progress, it should be in good shape. As a reviewer, I am not authorized to approve pull requests; this feedback is intended to help improve the code quality for subsequent reviews and merging decisions.

[gemini-code-assist]

It appears the unused lint rule (unused = { level = "deny", priority = -1 }) has been commented out. Could you clarify the reason for this change?

If this is a temporary measure to facilitate the work-in-progress refactoring, it's understandable. However, it's crucial to ensure this lint is re-enabled before merging to prevent potentially unused code from being overlooked. Maintaining a strict policy on unused code helps keep the codebase clean and maintainable.

[gemini-code-assist]

The documentation for Endpoint::address (line 31) appears to be a copy-paste from Endpoint::hostname. It should read something like: /// Create an endpoint from a socket address.

[gemini-code-assist]

The tracing span for the endpoint uses endpoint = name. Other similar client spawning functions (e.g., dnp3/src/tcp/master/client.rs line 72, dnp3/src/tcp/outstation/client.rs line 85) use endpoint = ?name. For consistency and to leverage debug formatting for structured log fields, ?name is generally preferred.