fix: safe type assertions in CloseNotify() and Hijack()

[jassus213]

Problem

Similar to #4460 (fixed for Flush() in #4479), Hijack() and CloseNotify() perform direct type assertions that panic when the underlying writer doesn't implement the interface.

When gin is used with http.TimeoutHandler or any middleware that wraps http.ResponseWriter with a type that doesn't implement http.Hijacker or http.CloseNotifier, calling Hijack() or CloseNotify() causes a runtime panic:

// panics if underlying writer is not http.CloseNotifier                                                                                                                                    
return w.ResponseWriter.(http.CloseNotifier).CloseNotify()                       
                                                          
// panics if underlying writer is not http.Hijacker       
return w.ResponseWriter.(http.Hijacker).Hijack()                                                                                                                                            

Fix

Replace direct assertions with checked assertions - the same pattern already used in Flush():

• CloseNotify() returns nil when unsupported (http.CloseNotifier is deprecated since Go 1.11; callers should use Request.Context().Done())
• Hijack() returns errHijackNotSupported, consistent with the existing errHijackAlreadyWritten pattern

The capability check in Hijack() now runs before mutating w.size, so Written() stays false on the error path.

Tests

Added TestResponseWriterOptionalInterfaceFallbacks with a mockNonHijackerWriter (implements only http.ResponseWriter) verifying that Flush(), CloseNotify(), and Hijack() all degrade gracefully without panicking.

Closes #4638.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.37%. Comparing base (3dc1cd6) to head (3317f91).
⚠️ Report is 275 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4639      +/-   ##
==========================================
- Coverage   99.21%   98.37%   -0.84%     
==========================================
  Files          42       48       +6     
  Lines        3182     3148      -34     
==========================================
- Hits         3157     3097      -60     
- Misses         17       42      +25     
- Partials        8        9       +1     

Flag	Coverage Δ
	?
--ldflags="-checklinkname=0" -tags sonic	98.36% <100.00%> (?)
-tags go_json	98.30% <100.00%> (?)
-tags nomsgpack	98.35% <100.00%> (?)
go-1.18	?
go-1.19	?
go-1.20	?
go-1.21	?
go-1.25	98.37% <100.00%> (?)
go-1.26	98.37% <100.00%> (?)
macos-latest	98.37% <100.00%> (-0.84%)	⬇️
ubuntu-latest	98.37% <100.00%> (-0.84%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Wsuffy]

Hit this exact panic today when adding http.TimeoutHandler to our gin setup. Looked at the PR and the fix solves it - safe assertion + graceful fallback, consistent with how Flush() is handled.

[Qodo-Free-For-OSS]

Hi, responseWriter.CloseNotify() now returns nil when the underlying ResponseWriter doesn’t implement http.CloseNotifier, but Context.Stream() unconditionally selects on that channel to detect disconnects. A nil channel disables that select case permanently, so Stream will not stop on client disconnect and can loop indefinitely until step() returns false.

Severity: action required | Category: correctness

How to fix: Fallback to request context

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

responseWriter.CloseNotify() now returns nil if the underlying writer does not implement http.CloseNotifier. Context.Stream() unconditionally selects on clientGone := w.CloseNotify(). In Go, receiving from a nil channel is a permanently-blocked case, so the disconnect branch never triggers and Stream() can’t terminate on disconnect.

Issue Context

This PR targets wrappers (e.g., http.TimeoutHandler) that may remove optional interfaces from the underlying http.ResponseWriter. Those same wrappers can now cause Stream() to spin until step() returns false, even if the client is gone.

Fix Focus Areas

• context.go[1326-1342]

Implementation notes

• Keep CloseNotify()’s new safe behavior.
• Update Context.Stream() to handle the nil CloseNotify channel by adding a fallback disconnect signal (preferably c.Request.Context().Done() when c.Request != nil). Example structure:
  • compute clientGone := c.Writer.CloseNotify()
  • in the loop select { case <-clientGone: return true; case <-c.Request.Context().Done(): return true; default: ... }
  • if c.Request can be nil in any code paths, guard accordingly (e.g., only add the Done() case when non-nil).

---

Found by Qodo code review. FYI, Qodo is free for open-source.

[jassus213]

@Qodo-Free-For-OSS

Valid point.

CloseNotify() now returns nil when http.CloseNotifier is unsupported, while Context.Stream() previously selected only on that channel. In wrapped-writer scenarios (e.g. http.TimeoutHandler), this can disable disconnect detection in Stream().

Fixed in this PR by adding a fallback to c.Request.Context().Done() (guarded with c.Request != nil) and keeping the safe CloseNotify() behavior unchanged. Added a test for this path as well.

[jassus213]

@appleboy - gentle ping on safe assertions for Hijack/CloseNotify + Stream disconnect fallback. Would appreciate a review when you can.

Thanks.