[leehainuo]

Description

responseWriter.Hijack() currently changes the writer state from noWritten (-1) to 0 before the underlying http.Hijacker.Hijack() call succeeds.

If the wrapped ResponseWriter implements http.Hijacker but the actual hijack operation returns an error, Gin leaves the response marked as written even though the connection was not hijacked and no HTTP response was written.

As a result, the handler can no longer send a normal fallback response with the intended status code.

For example:

_, _, err := c.Writer.Hijack()
if err != nil {
    c.String(http.StatusInternalServerError, "could not establish tunnel")
    return
}

Before this fix, the failed hijack makes Written() return true, so the subsequent 500 status is ignored and the client may receive 200 OK instead.

Reproduction

This can be reproduced with a real HTTP/2 request when a middleware ResponseWriter wrapper implements http.Hijacker and delegates the operation through http.ResponseController.

The underlying HTTP/2 response writer does not support traditional connection hijacking, so the actual Hijack() call returns an error. The handler should still be able to send a normal HTTP error response after that failure.

Before the fix:

Protocol: HTTP/2.0
before Hijack:        written=false size=-1 status=200
Hijack failed
after failed Hijack: written=true  size=0  status=200
fallback requested:  500
client received:      200 OK

After the fix:

Protocol: HTTP/2.0
before Hijack:        written=false size=-1 status=200
Hijack failed
after failed Hijack: written=false size=-1 status=200
fallback requested:  500
client received:      500 Internal Server Error

Standard library behavior

This behavior is also consistent with Go's net/http tests.

In TestResponseControllerHijack,
the HTTP/2 case expects ResponseController.Hijack() to return an error and
then continues using the same ResponseWriter to produce a normal HTTP
response.

This matches the expected behavior here: a failed hijack should not make an
otherwise unwritten response unusable for an HTTP fallback.

Root cause

The writer is marked as written before delegating to the underlying hijacker:

if w.size < 0 {
    w.size = 0
}

return hijacker.Hijack()

If the underlying Hijack() fails, that state change remains even though hijacking never succeeded.

Fix

Call the underlying Hijack() first and only transition an unwritten response to the written state after a successful hijack:

conn, rw, err := hijacker.Hijack()
if err == nil && w.size < 0 {
    w.size = 0
}

return conn, rw, err

This preserves the existing behavior for successful hijacks and for responses that were already committed before the hijack attempt.

Tests

Added a regression test that verifies:

• a failed underlying Hijack() leaves an unwritten response in the noWritten state;
• Written() remains false and Size() remains -1;
• the handler can still send a normal 500 Internal Server Error fallback response.

Locally validated with:

go test . -run '^TestResponseWriterHijack' -count=1 -v
go test . -count=1
go test ./... -count=1
go test -race . -count=1
git diff --check

Pull Request Checklist

• 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. (Not applicable: this is a bug fix and introduces no new feature.)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.65%. Comparing base (0f09c3a) to head (412f843).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4876   +/-   ##
=======================================
  Coverage   98.65%   98.65%           
=======================================
  Files          48       48           
  Lines        3188     3189    +1     
=======================================
+ Hits         3145     3146    +1     
  Misses         43       43           

Flag	Coverage Δ
--ldflags="-checklinkname=0" -tags sonic	98.64% <100.00%> (+<0.01%)	⬆️
-tags go_json	98.57% <100.00%> (+<0.01%)	⬆️
-tags nomsgpack	98.63% <100.00%> (+<0.01%)	⬆️
go-1.26	98.33% <100.00%> (+<0.01%)	⬆️
go-1.27	98.67% <100.00%> (+<0.01%)	⬆️
macos-latest	98.65% <100.00%> (+<0.01%)	⬆️
ubuntu-latest	98.65% <100.00%> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

[wahajahmed010]

Clean fix. The order swap (Hijack() first, then transition w.size only on success) is correct and matches Go's own net/http TestResponseControllerHijack semantics for the HTTP/2 fallback case described in the PR.

A few minor observations:

1. Symmetry consideration. The condition err == nil && w.size < 0 handles two distinct cases: (a) successful hijack on an unwritten response (transition to written), (b) successful hijack on an already-written response (no-op). That is correct. The only behavior change is on the previously-broken case (c) failed hijack on an unwritten response, where the writer now stays unwritten. Good.

2. Test coverage. The new test covers failingHijacker + http.Error fallback path. Consider also asserting the inverse: a successful hijack on an unwritten response still transitions Written() to true and Size() to 0 (the original happy-path behavior). That guard prevents a future "always skip the size update" regression from sneaking back in.

3. Doc comment. Hijack()'s godoc says nothing about what happens to the writer state on failure. A one-liner along the lines of "On error, the response writer is left in its prior state so the caller can emit a fallback HTTP response" would help future middleware authors avoid the trap this PR fixes.

LGTM once #2 (or equivalent) is added.

[leehainuo]

@wahajahmed010 Thanks for the review. I've added the successful hijack assertion for Written() == true and Size() == 0, and also clarified the failure behavior in the Hijack() doc comment. Ready for another look when you have a chance.

[leehainuo]

Hi @wahajahmed010,

I've squashed the commits into a single commit to follow the repository's contribution guidelines.

The fix, regression tests, and documentation updates are all preserved.

Thanks again for your review! I'd appreciate another look when you have a chance.