feat(http): implement Hijack method in ResponseRecorder and enhance e… #32
Conversation
…rror handling for chunked responses
There was a problem hiding this comment.
Pull request overview
This PR enhances HTTP response handling by implementing connection hijacking support and improving error handling for streaming responses. The changes enable proper cleanup of client connections when upstream errors occur during response streaming.
Changes:
- Implemented
http.Hijackerinterface inResponseRecorderto enable direct connection access - Added
wrapUpstreamErrorhelper to gracefully close connections on upstream errors - Refactored chunked response handling logic for better maintainability
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/x/http/response_recorder.go | Implements Hijack method to support connection hijacking through the wrapped response writer |
| server/server.go | Adds connection cleanup on upstream errors and refactors chunked response detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if hj, ok := r.ResponseWriter.(http.Hijacker); ok { | ||
| return hj.Hijack() | ||
| } | ||
| return nil, nil, http.ErrHijacked |
There was a problem hiding this comment.
The error return should be http.ErrNotSupported instead of http.ErrHijacked. http.ErrHijacked indicates the connection was already hijacked, while http.ErrNotSupported correctly indicates the underlying ResponseWriter doesn't implement the Hijacker interface.
| return nil, nil, http.ErrHijacked | |
| return nil, nil, http.ErrNotSupported |
| var _ http.Hijacker = (*ResponseRecorder)(nil) | ||
|
|
||
| type ResponseRecorder struct { | ||
| http.Hijacker |
There was a problem hiding this comment.
Embedding http.Hijacker as an interface field is unnecessary since the Hijack method is explicitly implemented below. This embedded field serves no purpose and should be removed to avoid confusion.
| http.Hijacker |
| // handle copy error | ||
| if err != nil && !errors.Is(err, io.EOF) { | ||
| // abort ? continue ? | ||
| // quickabort ? continue ? |
There was a problem hiding this comment.
Corrected spacing in comment from 'quickabort' to 'quick abort'.
| // quickabort ? continue ? | |
| // quick abort ? continue ? |
This pull request enhances HTTP response handling, particularly around connection management and error handling when streaming responses. The main improvements include implementing the
http.Hijackerinterface forResponseRecorder, better handling of upstream errors for chunked or unknown-length responses, and refactoring related logic in the server handler.HTTP Response Handling Improvements:
http.Hijackerinterface forResponseRecorderinpkg/x/http/response_recorder.go, allowing direct access to the underlying network connection when needed. [1] [2]wrapUpstreamErrorhelper function inserver/server.goto close client connections gracefully when upstream response errors occur, specifically for chunked or unknown-length bodies.Server Handler Logic Updates:
buildHandlerto use the newwrapUpstreamErrorfunction, ensuring connections are properly closed on copy errors for chunked or unspecified content-length responses.hasChunkedvariable and updating logging for chunked responses.