feat: unwrap resp for better deadline/flush SSE support #613
Conversation
WalkthroughThe changes in this pull request enhance the Server-Sent Events (SSE) functionality by introducing multiple router options and improving HTTP response writer handling. The Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #613 +/- ##
==========================================
+ Coverage 92.84% 92.88% +0.03%
==========================================
Files 22 22
Lines 4810 4834 +24
==========================================
+ Hits 4466 4490 +24
Misses 299 299
Partials 45 45 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
sse/sse_test.go (1)
119-122: LGTM: Good addition to error handling testsThe new test case for write deadline errors is a valuable addition. It ensures that the system handles these errors gracefully without panicking, which is crucial for robust SSE implementation.
Consider adding a comment explaining the purpose of this test case for better clarity:
// Test inability to set write deadline due to error doesn't panic + // This simulates a scenario where setting a write deadline fails w = &DummyWriter{deadlineErr: errors.New("whoops")} req, _ = http.NewRequest(http.MethodGet, "/sse", nil) api.Adapter().ServeHTTP(w, req)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/sse/main.go (2 hunks)
- sse/sse.go (3 hunks)
- sse/sse_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
sse/sse_test.go (5)
31-32: LGTM: Enhanced error simulation capabilityThe addition of the
deadlineErrfield to theDummyWriterstruct is a good improvement. It allows for more comprehensive testing by simulating errors related to setting write deadlines, which is crucial for robust SSE implementation.
45-47: LGTM: Proper implementation of unwrappingThe
Unwrapmethod is a great addition. It follows Go's conventions for unwrapping and allows access to the underlyingWrappedDeadliner. This change supports better handling of HTTP response writers, which is one of the main objectives of this PR.
49-52: LGTM: Well-designed wrapper for deadline handlingThe
WrappedDeadlinerstruct is a well-thought-out addition. By embeddinghttp.ResponseWriterand including adeadlineErrfield, it provides a clean way to handle deadline errors while maintaining compatibility with the standard library. This design supports the PR's goal of improving SSE support and error handling.
54-55: LGTM: Proper implementation of SetWriteDeadlineThe
SetWriteDeadlinemethod is correctly implemented. It allows for simulating deadline setting errors, which is crucial for testing error handling in SSE implementations. This method aligns well with the PR's objective of improving deadline handling and flush support for SSE across various routers.
Line range hint
31-122: Overall assessment: Excellent improvements to SSE testingThe changes in this file significantly enhance the testing capabilities for SSE functionality, particularly in error handling scenarios. The additions to the
DummyWriter, the newWrappedDeadliner, and the extended test cases all contribute to a more robust and comprehensive test suite.These improvements align perfectly with the PR objectives of enhancing SSE support and improving HTTP response writer handling. They provide better coverage for potential edge cases and error scenarios, which will lead to more reliable SSE implementations across different routers.
Great job on these enhancements!
| var flusher http.Flusher | ||
| flushCheck := bw | ||
| for { | ||
| if f, ok := flushCheck.(http.Flusher); ok { | ||
| flusher = f | ||
| break | ||
| } | ||
| if u, ok := flushCheck.(unwrapper); ok { | ||
| flushCheck = u.Unwrap() | ||
| } else { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| var deadliner writeDeadliner | ||
| deadlineCheck := bw | ||
| for { | ||
| if d, ok := deadlineCheck.(writeDeadliner); ok { | ||
| deadliner = d | ||
| break | ||
| } | ||
| if u, ok := deadlineCheck.(unwrapper); ok { | ||
| deadlineCheck = u.Unwrap() | ||
| } else { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated unwrapping logic into a helper function
The unwrapping logic to obtain http.Flusher and writeDeadliner interfaces from the response writer is duplicated in two places. This duplication can make the code harder to maintain and more error-prone. Consider refactoring this into a reusable helper function to improve code maintainability and reduce duplication.
You could create a generic helper function like this:
// Helper function to unwrap the response writer until the desired interface is found.
func getInterface[T any](bw http.ResponseWriter) T {
check := bw
for {
if res, ok := check.(T); ok {
return res
}
if u, ok := check.(unwrapper); ok {
check = u.Unwrap()
} else {
var zero T
return zero
}
}
}Then replace the unwrapping loops with calls to this helper function:
- var flusher http.Flusher
- flushCheck := bw
- for {
- if f, ok := flushCheck.(http.Flusher); ok {
- flusher = f
- break
- }
- if u, ok := flushCheck.(unwrapper); ok {
- flushCheck = u.Unwrap()
- } else {
- break
- }
- }
+ flusher := getInterface[http.Flusher](bw)And similarly for deadliner:
- var deadliner writeDeadliner
- deadlineCheck := bw
- for {
- if d, ok := deadlineCheck.(writeDeadliner); ok {
- deadliner = d
- break
- }
- if u, ok := deadlineCheck.(unwrapper); ok {
- deadlineCheck = u.Unwrap()
- } else {
- break
- }
- }
+ deadliner := getInterface[writeDeadliner](bw)
This PR adds support for unwrapping HTTP response writers similar to how Go's built-in
http.ResponseControllerhandles it to enable better support for setting write deadlines and flushing during Server Sent Events handling in various routers. This fixes some issues with Chi and Gin, and updates the SSE example to be able to run with either router to show that it works.Fixes #491. Related to go-chi/chi#947.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
DummyWriterto cover new error scenarios related to write deadlines.