response_writer: don't panic on Hijack/CloseNotify when wrapper unsupported#4645
response_writer: don't panic on Hijack/CloseNotify when wrapper unsupported#4645SAY-5 wants to merge 2 commits into
Conversation
…ported Closes gin-gonic#4638. http.TimeoutHandler's writer doesn't implement http.Hijacker/CloseNotifier; mirror Flush's graceful degradation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4645 +/- ##
==========================================
- Coverage 99.21% 98.37% -0.84%
==========================================
Files 42 48 +6
Lines 3182 3143 -39
==========================================
- Hits 3157 3092 -65
- Misses 17 42 +25
- Partials 8 9 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The only failing check is |
There was a problem hiding this comment.
Pull request overview
This PR updates Gin’s responseWriter wrapper to gracefully degrade (instead of panicking) when the underlying http.ResponseWriter does not implement http.Hijacker or http.CloseNotifier, aligning behavior with the existing Flush() pattern.
Changes:
- Replace direct type assertions in
Hijack()with anokcheck and returnhttp.ErrNotSupportedwhen unsupported. - Replace direct type assertions in
CloseNotify()with anokcheck and returnnilwhen unsupported. - Update
TestResponseWriterHijackto expect non-panicking behavior for unsupported interfaces.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| response_writer.go | Adds safe interface checks for Hijack()/CloseNotify() to avoid panics with wrappers like http.TimeoutHandler. |
| response_writer_test.go | Updates tests to validate graceful degradation (no panic) when the underlying writer lacks Hijacker/CloseNotifier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| w.size = 0 | ||
| } | ||
| return w.ResponseWriter.(http.Hijacker).Hijack() | ||
| hijacker, ok := w.ResponseWriter.(http.Hijacker) | ||
| if !ok { | ||
| return nil, nil, http.ErrNotSupported |
| require.ErrorIs(t, err, http.ErrNotSupported) | ||
| assert.True(t, w.Written()) |
Closes #4638.
http.TimeoutHandler's timeoutWriter doesn't implementhttp.Hijacker/http.CloseNotifier, direct type assertion panics. Match the existingFlush()graceful-degradation pattern (#4479): returnhttp.ErrNotSupportedfromHijackandnilfromCloseNotify.