fix: use safe type assertions in Hijack and CloseNotify to prevent pa…

[odlev]

Pull Request Checklist

Please ensure your pull request meets the following requirements:

• Open your pull request against the master branch.
• All tests pass in available continuous integration systems (e.g., GitHub Actions).
• Tests are added or modified as needed to cover code changes.
• If the pull request introduces a new feature, the feature is documented in the docs/doc.md.

---

What

responseWriter.Hijack() and responseWriter.CloseNotify() use unsafe type assertions on the underlying http.ResponseWriter, which causes panics when the writer doesn't implement http.Hijacker or http.CloseNotifier — for example when using httptest.NewRecorder() through gin.CreateTestContext().

Why

Flush() and Pusher() already use the safe value, ok := iface.(Type) pattern. Hijack() and CloseNotify() are the only methods that still do bare type assertions, which is inconsistent and crashes in test environments.

Changes

• Hijack(): returns a descriptive error (errHijackNotSupported) instead of panicking when the underlying writer doesn't implement http.Hijacker
• CloseNotify(): returns nil channel instead of panicking when the underlying writer doesn't implement http.CloseNotifier
• Updated TestResponseWriterHijack to expect error return instead of panic
• Added TestHijackWithoutHijacker, TestCloseNotifyWithCloseNotifier, TestCloseNotifyWithoutCloseNotifier

How to verify

c, _ := gin.CreateTestContext(httptest.NewRecorder())
// Before: panics
// After: Hijack() returns errHijackNotSupported, CloseNotify() returns nil
_, _, err := c.Writer.Hijack()
// err == errHijackNotSupported

ch := c.Writer.CloseNotify()
// ch == nil

Fixes #2970

p.s.

codecov/project failure is a stale base comparison issue (275 commits behind), patch coverage is 100%.

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4628      +/-   ##
==========================================
- Coverage   99.21%   98.37%   -0.84%     
==========================================
  Files          42       48       +6     
  Lines        3182     3142      -40     
==========================================
- Hits         3157     3091      -66     
- 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.

[afurm]

For CloseNotify(), returning nil when http.CloseNotifier is not supported means callers can't distinguish between 'no channel available' and 'a channel that will never fire'. A nil channel is also problematic for range/choke patterns. Would a closed channel (make(chan bool) immediately closed, or a perpetually open channel) be more semantically correct here?

[odlev]

Nice point). Returning nil is ambiguous and problematic in range patterns. Changed to return [make(chan bool)] - a channel that is never closed and never fires. This makes the "not supported" semantic explicit: callers in a select will simply never receive, same as nil but without the edge cases.

[afurm]

Fair point — a channel that never fires does make the "not supported" semantic explicit. Though for callers who range over this channel, an always-blocking channel could stall their loop indefinitely. Worth adding a comment clarifying the contract so consumers don't misuse it as a "done" signal.

[BootstrapperSBL]

Small suggestion to close @afurm's ambiguity point without further code churn: surface the "never fires when not supported" semantics directly in the godoc. Something like:

// CloseNotify implements the http.CloseNotifier interface.
//
// If the underlying ResponseWriter doesn't implement http.CloseNotifier
// (e.g. httptest.NewRecorder), the returned channel will never fire.
// Use Request.Context().Done() to observe client disconnects instead.
//
// Deprecated: the CloseNotifier interface predates Go's context package.
// New code should use Request.Context instead.

Makes the contract self-documenting for anyone who lands on this via IDE hover, and keeps the impl unchanged.

[odlev]

added detailed comment

[fgkgerer]

这是一封自动回复邮件。已经收到您的来信，我会尽快回复。