Skip to content

Add warning when writing response body multiple times (fixes #4477)#4483

Open
raju-mechatronics wants to merge 6 commits intogin-gonic:masterfrom
raju-mechatronics:master
Open

Add warning when writing response body multiple times (fixes #4477)#4483
raju-mechatronics wants to merge 6 commits intogin-gonic:masterfrom
raju-mechatronics:master

Conversation

@raju-mechatronics
Copy link
Contributor

Add warning for multiple response writes

Fixes #4477

Description

Adds a warning when the response body is written multiple times in the same request. This prevents silent concatenation of multiple JSON/XML responses, which often produces invalid output.

Changes

  • Added warning in Context.Render() when response body already written
  • Consistent with existing header write warning behavior
  • Non-breaking change (debug mode only)

Example

router.GET("/example", func(c *gin.Context) {
    c.JSON(200, gin.H{"first": "response"})
    c.JSON(200, gin.H{"second": "response"})
})

Output:

[GIN-debug] [WARNING] Response body already written. Attempting to write again with status code 200

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.

@codecov
Copy link

codecov bot commented Dec 28, 2025

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4483      +/-   ##
==========================================
- Coverage   99.21%   98.80%   -0.42%     
==========================================
  Files          42       47       +5     
  Lines        3182     3092      -90     
==========================================
- Hits         3157     3055     -102     
- Misses         17       27      +10     
- Partials        8       10       +2     
Flag Coverage Δ
?
--ldflags="-checklinkname=0" -tags sonic 98.79% <100.00%> (?)
-tags go_json 98.66% <100.00%> (?)
-tags nomsgpack 98.72% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 ?
go-1.25 98.80% <100.00%> (?)
go-1.26 98.73% <100.00%> (?)
macos-latest 98.80% <100.00%> (-0.42%) ⬇️
ubuntu-latest 98.73% <100.00%> (-0.48%) ⬇️

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.

@raju-mechatronics
Copy link
Contributor Author

@appleboy can you please take a look at this pr

@Nurysso
Copy link
Contributor

Nurysso commented Jan 1, 2026

Hey nice addition but when the warning fires the code still continues to render, which means the response could still gets corrupted, right? Shouldn't we return after the warning to actually prevent the invalid output?

@raju-mechatronics
Copy link
Contributor Author

Hey nice addition but when the warning fires the code still continues to render, which means the response could still gets corrupted, right? Shouldn't we return after the warning to actually prevent the invalid output?

I was waiting for reviewer response on this but it's for backward compatibility. Watch #4477 for details. It's seems multiple response is allowed in gin.

@appleboy appleboy added this to the v1.12 milestone Jan 2, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a debug-mode warning when a handler attempts to write a response body more than once during the same request, addressing silent concatenation (e.g., multiple JSON/XML writes) that can produce invalid output.

Changes:

  • Add a debug warning in Context.Render() when a second render occurs after the response has already been written.
  • Exempt code == -1 (used for SSE/stream-like cases) and sse.Event from the warning.
  • Add tests to validate the warning for multiple JSON writes and ensure no warning for repeated SSE writes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
context.go Emits a debug warning when rendering after the response is already written, with special-casing for SSE/-1 code.
context_test.go Adds tests covering the new warning behavior for JSON and SSE.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1160 to +1165
if c.Writer.Written() && IsDebugging() {
// Skip warning for SSE and streaming responses (status code -1)
if code != -1 {
if _, ok := r.(sse.Event); !ok {
debugPrint("[WARNING] Response body already written. Attempting to write again with status code %d", code)
}
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.Writer.Written() becomes true as soon as headers are written (Size()==0), not only after body bytes are written. This can emit a false warning on the first body write if a handler/middleware flushes headers early (e.g., calls c.Writer.WriteHeaderNow()/Flush()) and then renders JSON. To specifically detect multiple body writes (as the message says), consider checking c.Writer.Size() > 0 (or equivalent) instead of Written().

Copilot uses AI. Check for mistakes.
}

if c.Writer.Written() && IsDebugging() {
// Skip warning for SSE and streaming responses (status code -1)
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment says “Skip warning for SSE and streaming responses (status code -1)”, but code == -1 is also used for non-streaming renders like render.Redirect, and the logic also has a separate sse.Event type check. Please clarify the comment (and/or consolidate the conditions) so future readers understand why -1 and sse.Event are treated specially.

Suggested change
// Skip warning for SSE and streaming responses (status code -1)
// Skip this warning when:
// - status code is -1, which is used for special/streaming-style renders (including some
// non-streaming helpers like redirects) where multiple writes are expected, and
// - the renderer is an SSE event, since Server-Sent Events are sent as a stream.

Copilot uses AI. Check for mistakes.
Comment on lines +1404 to +1407
oldMode := os.Getenv("GIN_MODE")
defer os.Setenv("GIN_MODE", oldMode)
SetMode(DebugMode)

Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests call SetMode(DebugMode) but never restore the previous gin mode, which can leak global state into later tests (the suite defaults to TestMode). Capture the prior Mode() (or assume TestMode) and defer SetMode(...) to restore it after the test.

Copilot uses AI. Check for mistakes.
Comment on lines +1404 to +1406
oldMode := os.Getenv("GIN_MODE")
defer os.Setenv("GIN_MODE", oldMode)
SetMode(DebugMode)
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oldMode := os.Getenv("GIN_MODE") / defer os.Setenv(...) is redundant here because the test never modifies the environment variable, and SetMode(...) does not read GIN_MODE after init. Consider removing the env save/restore and instead restoring the previous gin mode via Mode() + SetMode.

Copilot uses AI. Check for mistakes.
Comment on lines +1423 to +1426
oldMode := os.Getenv("GIN_MODE")
defer os.Setenv("GIN_MODE", oldMode)
SetMode(DebugMode)

Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test calls SetMode(DebugMode) but never restores the previous gin mode, which can leak global state into later tests. Please defer SetMode(...) to restore the mode (usually TestMode) after the assertions complete.

Copilot uses AI. Check for mistakes.
Comment on lines +1423 to +1425
oldMode := os.Getenv("GIN_MODE")
defer os.Setenv("GIN_MODE", oldMode)
SetMode(DebugMode)
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the previous test, the GIN_MODE env save/restore is unused because the test never changes GIN_MODE, and SetMode does not consult the env var after startup. Prefer removing this and restoring the prior gin mode via Mode()/SetMode instead.

Copilot uses AI. Check for mistakes.
@appleboy
Copy link
Member

move to the next milestone

@appleboy appleboy modified the milestones: v1.12, v1.x, v1.13 Feb 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add warning for multiple response writes

4 participants