Added logging in TCP and improved exception handling (removed logging because of other PR)#101
Open
Added logging in TCP and improved exception handling (removed logging because of other PR)#101
Conversation
📝 WalkthroughWalkthroughReworks TcpServer accept loop to be interruption-aware, adds socket timeouts (server + client), decouples accept from client handling with new helpers, centralizes best-effort 500 handling and quiet socket/handler cleanup, and updates tests to mock connected sockets and assert response contents. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Socket
participant Server as TcpServer
participant Factory as ConnectionFactory
participant Handler as ConnectionHandler
participant Stream as OutputStream
Client->>Server: connect()
Server->>Server: accept() (timeout-driven, interruption-aware)
Server->>Factory: createConnection(clientSocket)
Factory-->>Handler: ConnectionHandler
Server->>Server: startClientTask(clientSocket)
Server->>Handler: processRequest() (try-with-resources)
alt normal response
Handler->>Stream: write response
Stream-->>Client: response bytes
else internal error
Server->>Server: handleInternalServerError(clientSocket)
Server->>Stream: write 500 response (best-effort)
Stream-->>Client: 500 response bytes
end
Server->>Client: close (closeQuietly / try-with-resources)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/TcpServer.java (1)
29-29: Consider TLS for production deployments with sensitive data.The static analysis tool flagged that
ServerSockettransmits data unencrypted. If this server handles sensitive information in production, consider usingSSLServerSocketFactoryto create an encrypted socket. For internal services or when TLS termination is handled by a load balancer/reverse proxy, plain TCP may be acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` at line 29, The code currently creates an unencrypted listener with new ServerSocket(port) in the TcpServer class; for production handling sensitive data replace that with an SSL/TLS server socket created via SSLServerSocketFactory or an SSLContext (e.g., SSLServerSocketFactory.getDefault() or sslContext.getServerSocketFactory().createServerSocket(port)), load a keystore (KeyManagerFactory) and configure protocols/cipher suites and client-auth as required, and make this behavior conditional on configuration so non-TLS local/testing runs can still use the existing path; update the accept/connection-handling code to use the resulting SSLServerSocket/SSLSocket APIs and ensure proper certificate/key management and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 29-43: The accept() loop uses
Thread.currentThread().isInterrupted() but accept() blocks and won't observe
interrupts; before the loop call serverSocket.setSoTimeout(timeoutMillis) to
make accept() periodically throw SocketTimeoutException, then catch
SocketTimeoutException around serverSocket.accept() and simply continue to
re-check Thread.currentThread().isInterrupted(), while preserving other
exceptions (log and break/cleanup); alternatively ensure your shutdown logic
closes the ServerSocket from another thread to unblock
serverSocket.accept()—refer to ServerSocket, serverSocket.accept(),
serverSocket.setSoTimeout(...), and SocketTimeoutException handling in your
changes.
---
Nitpick comments:
In `@src/main/java/org/example/TcpServer.java`:
- Line 29: The code currently creates an unencrypted listener with new
ServerSocket(port) in the TcpServer class; for production handling sensitive
data replace that with an SSL/TLS server socket created via
SSLServerSocketFactory or an SSLContext (e.g.,
SSLServerSocketFactory.getDefault() or
sslContext.getServerSocketFactory().createServerSocket(port)), load a keystore
(KeyManagerFactory) and configure protocols/cipher suites and client-auth as
required, and make this behavior conditional on configuration so non-TLS
local/testing runs can still use the existing path; update the
accept/connection-handling code to use the resulting SSLServerSocket/SSLSocket
APIs and ensure proper certificate/key management and error handling.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/TcpServer.java (1)
31-33:⚠️ Potential issue | 🟠 Major
accept()still blocks shutdown despite interrupt-aware loop.Line 31 checks interruption, but Line 32 can block indefinitely on
accept(), so the loop may never re-check the flag during shutdown.Suggested fix
import java.net.ServerSocket; import java.net.Socket; +import java.net.SocketTimeoutException; ... try (ServerSocket serverSocket = new ServerSocket(port)) { + serverSocket.setSoTimeout(1000); while (!Thread.currentThread().isInterrupted()) { - Socket clientSocket = serverSocket.accept(); + Socket clientSocket; + try { + clientSocket = serverSocket.accept(); + } catch (SocketTimeoutException e) { + continue; // periodic interrupt check + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` around lines 31 - 33, The accept() call inside TcpServer's interrupt-aware loop blocks shutdown; make accept interruptible by configuring the ServerSocket with a timeout and handling SocketTimeoutException (e.g., call serverSocket.setSoTimeout(...)) or by closing serverSocket from the shutdown path so accept() throws, then catch the resulting exception and re-check Thread.currentThread().isInterrupted() to exit the loop; update the loop that calls serverSocket.accept() to catch the timeout/SocketException and continue/terminate appropriately.
🧹 Nitpick comments (1)
src/main/java/org/example/TcpServer.java (1)
55-57: Include throwable in warning log to preserve diagnostics.Line 56 logs only the message text; include the exception object to retain stack trace/context.
Suggested refactor
- } catch (IOException e) { - logger.log(Level.WARNING, () -> "Network error with client: " + e.getMessage()); + } catch (IOException e) { + logger.log(Level.WARNING, "Network error with client", e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` around lines 55 - 57, In the TcpServer IOException catch block update the logger call to pass the caught exception object so the stack trace and context are preserved: instead of logging only e.getMessage() inside the lambda, call logger.log(Level.WARNING, "Network error with client", e) (or equivalent logger.log overload) so the throwable is included; modify the catch(IOException e) handler where logger is used to include the exception parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 31-33: The accept() call inside TcpServer's interrupt-aware loop
blocks shutdown; make accept interruptible by configuring the ServerSocket with
a timeout and handling SocketTimeoutException (e.g., call
serverSocket.setSoTimeout(...)) or by closing serverSocket from the shutdown
path so accept() throws, then catch the resulting exception and re-check
Thread.currentThread().isInterrupted() to exit the loop; update the loop that
calls serverSocket.accept() to catch the timeout/SocketException and
continue/terminate appropriately.
---
Nitpick comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 55-57: In the TcpServer IOException catch block update the logger
call to pass the caught exception object so the stack trace and context are
preserved: instead of logging only e.getMessage() inside the lambda, call
logger.log(Level.WARNING, "Network error with client", e) (or equivalent
logger.log overload) so the throwable is included; modify the catch(IOException
e) handler where logger is used to include the exception parameter.
AnnaZiafar
requested changes
Feb 26, 2026
AnnaZiafar
left a comment
AnnaZiafar
left a comment
There was a problem hiding this comment.
Våra pr krockar med varandra. Har ett pr (create server logger) där jag skapar en centraliserad logger (xml) som kan användas av alla klasser. Lägger även till den i tcp-servern. Frågan är om man kanske vill köra med den eftersom alla klasser får enhetliga loggers då. Men jag tycker att du har fixat väldigt välstrukturerad och snygg felhantering. Vad tror ni om att bara mergea felhanteringen och använda den centralicerade loggern i den andra pr:en?
|
Jag ser att du utvecklat koden ordentligt och den ser proffsig ut med snygg dokumentation och all felhantering! Nice |
Author
|
Låter som ett jättebra förslag om du redan har fixat ett centraliserat system! |
…on handling and other changes
gitnes94
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/TcpServer.java (1)
61-63: Socket state check may give false confidence.
Socket.isConnected()returnstrueonce connected and doesn't reflect if the remote peer has closed the connection. The check!client.isConnected()will rarely befalseafter a successfulaccept(). Similarly,isClosed()only reflects local close operations.This is acceptable for a best-effort error response, but be aware these checks don't guarantee the write will succeed. The silent
IOExceptioncatch at line 74 handles actual failures appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` around lines 61 - 63, The socket state checks in handleInternalServerError (the calls to client.isClosed() and client.isConnected()) are misleading because isConnected() stays true after accept and isClosed() only reflects local state; remove the optimistic state gating and instead attempt the error response write unconditionally inside the existing try/catch so any IOException will be handled as before (or, at minimum, drop the isConnected() check and keep only isClosed() if you must guard locally); update the comment to state that writes are best-effort and rely on IOException handling in handleInternalServerError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 61-63: The socket state checks in handleInternalServerError (the
calls to client.isClosed() and client.isConnected()) are misleading because
isConnected() stays true after accept and isClosed() only reflects local state;
remove the optimistic state gating and instead attempt the error response write
unconditionally inside the existing try/catch so any IOException will be handled
as before (or, at minimum, drop the isConnected() check and keep only isClosed()
if you must guard locally); update the comment to state that writes are
best-effort and rely on IOException handling in handleInternalServerError.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/TcpServer.javasrc/test/java/org/example/TcpServerTest.java
… colleagues logging system
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/TcpServer.java (1)
23-24:⚠️ Potential issue | 🟠 Major
accept()can still block shutdown despite the interrupt check.The loop checks interruption, but
ServerSocket.accept()blocks and won’t notice thread interruption on its own. This can stall graceful shutdown.🔧 Proposed fix (timeout-based interrupt polling)
try (ServerSocket serverSocket = new ServerSocket(port)) { + serverSocket.setSoTimeout(1000); while (!Thread.currentThread().isInterrupted()) { - Socket clientSocket = serverSocket.accept(); + Socket clientSocket; + try { + clientSocket = serverSocket.accept(); + } catch (java.net.SocketTimeoutException ignored) { + continue; + } try { clientSocket.setSoTimeout(10000); Thread.ofVirtual().start(() -> handleClient(clientSocket)); - } catch (Exception _) { + } catch (Exception ignored) { closeQuietly(clientSocket); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` around lines 23 - 24, The accept() call can block shutdown—modify TcpServer to set a accept timeout on serverSocket (call serverSocket.setSoTimeout(...), e.g., 1000 ms) before entering the loop, then wrap Socket clientSocket = serverSocket.accept() in a try/catch that catches java.net.SocketTimeoutException and continues so the loop can re-check Thread.currentThread().isInterrupted(); also keep existing IOException handling for accept() and ensure any cleanup runs after the loop. This uses serverSocket, accept(), and Thread.currentThread().isInterrupted() from the current code to implement timeout-based interrupt polling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 43-45: The catch blocks in TcpServer (the catch(Exception _) and
the other block around lines 56-60) are swallowing exceptions; update those
handlers to emit a structured log and/or metric and preserve visibility of the
failure (e.g., log with a logger.error or metrics.increment including the
exception message and stack trace, and if appropriate rethrow or propagate a
wrapped RuntimeException). Locate the catch(Exception _) in class TcpServer (and
the second catch block referenced) and replace the empty handler with a
structured logging call that includes context (method name/connection id) and
the caught exception, and ensure you either rethrow or signal the central error
handler after logging.
---
Duplicate comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 23-24: The accept() call can block shutdown—modify TcpServer to
set a accept timeout on serverSocket (call serverSocket.setSoTimeout(...), e.g.,
1000 ms) before entering the loop, then wrap Socket clientSocket =
serverSocket.accept() in a try/catch that catches
java.net.SocketTimeoutException and continues so the loop can re-check
Thread.currentThread().isInterrupted(); also keep existing IOException handling
for accept() and ensure any cleanup runs after the loop. This uses serverSocket,
accept(), and Thread.currentThread().isInterrupted() from the current code to
implement timeout-based interrupt polling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/TcpServer.java`:
- Around line 64-66: The current guard in TcpServer that uses client.isClosed()
|| !client.isConnected() can miss sockets that have been shut down for output
(remote closed) because isConnected() remains true; update the guard in the
TcpServer method containing that snippet to also check client.isOutputShutdown()
(e.g., return early if client.isClosed() || client.isOutputShutdown() ||
!client.isConnected()) so you detect sockets that are unusable for writing
before proceeding.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/TcpServer.java (1)
25-25: Extract timeout literals to named constants.Line [25] and Line [48] use raw timeout values. Naming these improves readability and prevents drift when tuning.
Suggested refactor
public class TcpServer { + private static final int ACCEPT_TIMEOUT_MS = 1_000; + private static final int CLIENT_TIMEOUT_MS = 10_000; @@ - serverSocket.setSoTimeout(1000); + serverSocket.setSoTimeout(ACCEPT_TIMEOUT_MS); @@ - clientSocket.setSoTimeout(10000); + clientSocket.setSoTimeout(CLIENT_TIMEOUT_MS);Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` at line 25, Replace the raw timeout literals with named constants in the TcpServer class: define descriptive static final int constants (e.g., SERVER_SOCKET_TIMEOUT_MS and CLIENT_READ_TIMEOUT_MS or a single SOCKET_TIMEOUT_MS if both timeouts are the same) and use those constants instead of the hard-coded 1000 values where serverSocket.setSoTimeout(1000) and the other timeout at the referenced location are called; update any related comments to reflect the constant names so future tuning uses the constant rather than scattered magic numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/TcpServer.java`:
- Line 25: Replace the raw timeout literals with named constants in the
TcpServer class: define descriptive static final int constants (e.g.,
SERVER_SOCKET_TIMEOUT_MS and CLIENT_READ_TIMEOUT_MS or a single
SOCKET_TIMEOUT_MS if both timeouts are the same) and use those constants instead
of the hard-coded 1000 values where serverSocket.setSoTimeout(1000) and the
other timeout at the referenced location are called; update any related comments
to reflect the constant names so future tuning uses the constant rather than
scattered magic numbers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests