common/network-types/src/udp.rs (1)

304-305: Consider implementing socket splitting

The TODO comment suggests an important architectural improvement that could enhance socket management and reusability.

Would you like me to help design and implement the socket splitting functionality? I can create a GitHub issue to track this enhancement.

transport/session/src/manager.rs (2)

Line range hint 359-419: Consider enhancing error handling and race condition protection in session initiation

The session initiation logic could benefit from the following improvements:

1. Add more granular error handling for different failure scenarios (network issues, protocol mismatches, etc.)
2. Consider adding a mutex or similar synchronization primitive around session establishment to prevent potential race conditions when multiple sessions are being established simultaneously

 pub async fn new_session(&self, cfg: SessionClientConfig) -> crate::errors::Result<Session> {
+    // Add a mutex guard here to prevent race conditions
+    let _guard = self.session_establishment_lock.lock().await;
+
     let msg_sender = self.msg_sender.get().ok_or(SessionManagerError::NotStarted)?;
 
     let (tx_initiation_done, rx_initiation_done) = futures::channel::mpsc::unbounded();
@@ -419,6 +422,15 @@
             Either::Left((Err(e)), _) => {
-                error!(challenge = e.challenge, error = ?e, "the other party rejected the session initiation with error");
-                Err(TransportSessionError::Rejected(e.reason))
+                match e.reason {
+                    StartErrorReason::NoSlotsAvailable => {
+                        error!(challenge = e.challenge, "no slots available for session");
+                        Err(TransportSessionError::NoSlots)
+                    }
+                    StartErrorReason::ProtocolMismatch => {
+                        error!(challenge = e.challenge, "protocol version mismatch");
+                        Err(TransportSessionError::ProtocolMismatch)
+                    }
+                    _ => Err(TransportSessionError::Rejected(e.reason))
+                }
             }

---

519-519: Improve session logging consistency and metadata

The session logging could be enhanced by:

1. Using structured logging with consistent fields across all log messages
2. Including additional session metadata (e.g., protocol, capabilities)
3. Adding timing information for session establishment

-debug!(%peer, "got new session request, searching for a free session slot");
+debug!(
+    %peer,
+    protocol = ?session_req.target.protocol(),
+    capabilities = ?session_req.capabilities,
+    "got new session request, searching for a free session slot"
+);

-info!(%session_id, "new session established");
+info!(
+    %session_id,
+    protocol = ?session_req.target.protocol(),
+    capabilities = ?session_req.capabilities,
+    establishment_time_ms = %start_time.elapsed().as_millis(),
+    "new session established"
+);

Also applies to: 590-619

hoprd/rest-api/src/session.rs (2)

Line range hint 505-549: Ensure consistent listener management between TCP and UDP sessions.

In the UDP session handling, the listener is removed from open_listeners when the session closes:

// Once the Session closes, remove it from the list
open_listeners_clone.write().await.remove(&listener_id);

However, in the TCP case, this cleanup step is missing. This could lead to inconsistent state or resource leaks. Consider adding similar cleanup logic for TCP sessions to ensure consistent management across both protocols.

Apply this diff to add the cleanup logic for TCP sessions:

 state
     .open_listeners
     .write()
     .await
     .insert(ListenerId(protocol, bound_host), (target.clone(), jh));
+ 
+ // Remove the listener from the list when the task completes
+ let open_listeners_clone = state.open_listeners.clone();
+ let listener_id = ListenerId(protocol, bound_host);
+ hopr_async_runtime::prelude::spawn(async move {
+     jh.await.unwrap_or_else(|e| error!(error = %e, "TCP listener task failed"));
+     open_listeners_clone.write().await.remove(&listener_id);
+ });

 bound_host

---

788-791: Standardize logging format in error handling.

The logging statement has inconsistent formatting. Aligning it with the standard logging practices improves readability and consistency.

Apply this diff to standardize the logging format:

-            Err(error) => error!(
-                session_id = ?session_id,
-                %error,
-                "error during data transfer"
-            ),
+            Err(error) => {
+                error!(
+                    session_id = ?session_id,
+                    error = %error,
+                    "Error during data transfer"
+                );
+            },

Configuration used: .coderabbit.yaml
Review profile: CHILL

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6393
File: hoprd/rest-api/src/session.rs:91-91
Timestamp: 2024-11-12T10:55:50.205Z
Learning: The current implementation in `hoprd/rest-api/src/session.rs` assumes all sessions are TCP-like due to the missing implementation of UDP behavior. The commented-out code suggests an intention to support different session types based on `capabilities`.

Learnt from: Teebor-Choka
PR: hoprnet/hoprnet#6393
File: hoprd/rest-api/src/session.rs:91-91
Timestamp: 2024-11-12T10:56:01.095Z
Learning: The current implementation in `hoprd/rest-api/src/session.rs` assumes all sessions are TCP-like due to the missing implementation of UDP behavior. The commented-out code suggests an intention to support different session types based on `capabilities`.