feat: improve TCP framework based on architecture review #2
Conversation
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
Changes based on the architecture review report:
1. Rename heartbeat to idleTimeout for clarity
- The previous "heartbeat" was actually an idle timeout mechanism
- Added IdleTimeoutOption with clear documentation explaining it's
not a ping/pong heartbeat mechanism
- Removed deprecated HeartbeatOption (no backward compatibility)
2. Add message size validation with ErrMessageTooLarge
- Implemented limitedReader that returns ErrMessageTooLarge when
the message exceeds maxReadLength
- This provides clearer error messages when messages are too large
3. Improve Write method documentation
- Added detailed documentation for Write, WriteBlocking, WriteTimeout
- Documented ErrBufferFull with recommended handling strategies
- Added backpressure handling guidance in README
4. Add connection lifecycle logging
- Connection established/closed events logged at Info level
- Connection options logged at Debug level
- Server start/stop events logged at Info level
- Added ServerLoggerOption for custom logger
5. Add graceful shutdown timeout control
- Added ServerShutdownTimeoutOption for graceful shutdown
- Server waits for the timeout before stopping accept
Test coverage maintained at 3:1 ratio with new tests for:
- ErrMessageTooLarge validation
- limitedReader functionality
- ServerLoggerOption
1. Fix ShutdownTimeout to use select pattern - Use select with time.After instead of blocking sleep - Add shutdownNow channel to allow Close() to bypass timeout - This allows immediate shutdown when all connections are done 2. Add comment to limitedReader.reset - Explain why only remaining is reset (bufio.Reader maintains state) 3. Use errors.Is for context.Canceled comparison - More robust error comparison in conn.go Run method
- Use go-version-file to read Go version from go.mod - Use master as the only main branch - Add common gitignore entries (workspace, debug, profiling, etc.)
Zereker
added a commit
that referenced
this pull request
Dec 26, 2025
* feat: improve TCP framework based on architecture review
Changes based on the architecture review report:
1. Rename heartbeat to idleTimeout for clarity
- The previous "heartbeat" was actually an idle timeout mechanism
- Added IdleTimeoutOption with clear documentation explaining it's
not a ping/pong heartbeat mechanism
- Removed deprecated HeartbeatOption (no backward compatibility)
2. Add message size validation with ErrMessageTooLarge
- Implemented limitedReader that returns ErrMessageTooLarge when
the message exceeds maxReadLength
- This provides clearer error messages when messages are too large
3. Improve Write method documentation
- Added detailed documentation for Write, WriteBlocking, WriteTimeout
- Documented ErrBufferFull with recommended handling strategies
- Added backpressure handling guidance in README
4. Add connection lifecycle logging
- Connection established/closed events logged at Info level
- Connection options logged at Debug level
- Server start/stop events logged at Info level
- Added ServerLoggerOption for custom logger
5. Add graceful shutdown timeout control
- Added ServerShutdownTimeoutOption for graceful shutdown
- Server waits for the timeout before stopping accept
Test coverage maintained at 3:1 ratio with new tests for:
- ErrMessageTooLarge validation
- limitedReader functionality
- ServerLoggerOption
* fix: address review feedback on shutdown timeout and code quality
1. Fix ShutdownTimeout to use select pattern
- Use select with time.After instead of blocking sleep
- Add shutdownNow channel to allow Close() to bypass timeout
- This allows immediate shutdown when all connections are done
2. Add comment to limitedReader.reset
- Explain why only remaining is reset (bufio.Reader maintains state)
3. Use errors.Is for context.Canceled comparison
- More robust error comparison in conn.go Run method
* chore: update CI config and gitignore
- Use go-version-file to read Go version from go.mod
- Use master as the only main branch
- Add common gitignore entries (workspace, debug, profiling, etc.)
* fix: update golangci-lint config for v1.x compatibility
* fix: resolve build output conflict with example directory
---------
Co-authored-by: Zereker <Zereker@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
HeartbeatOptiontoIdleTimeoutOptionfor clarityErrMessageTooLargefor message size validationServerLoggerOptionandServerShutdownTimeoutOptiongo-version-filefrom go.modTest plan
-raceflaggo vetpasses