-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Add warning when writing response body multiple times (fixes #4477)#4483
Add warning when writing response body multiple times (fixes #4477)#4483raju-mechatronics wants to merge 5 commits intogin-gonic:masterfrom
Conversation
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
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
masterbranch. - 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 Report All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4483 +/- ## ========================================== - Coverage 99.21% 99.14% -0.07% ========================================== Files 42 46 +4 Lines 3182 3044 -138 ========================================== - Hits 3157 3018 -139 + Misses 17 16 -1 - Partials 8 10 +2
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:
|
|
@appleboy can you please take a look at this pr |
|
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 |
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. |
There was a problem hiding this 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) andsse.Eventfrom 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.
| 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) | ||
| } |
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
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().
| } | ||
|
|
||
| if c.Writer.Written() && IsDebugging() { | ||
| // Skip warning for SSE and streaming responses (status code -1) |
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
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.
| // 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. |
| oldMode := os.Getenv("GIN_MODE") | ||
| defer os.Setenv("GIN_MODE", oldMode) | ||
| SetMode(DebugMode) | ||
|
|
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
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.
| oldMode := os.Getenv("GIN_MODE") | ||
| defer os.Setenv("GIN_MODE", oldMode) | ||
| SetMode(DebugMode) |
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
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.
| oldMode := os.Getenv("GIN_MODE") | ||
| defer os.Setenv("GIN_MODE", oldMode) | ||
| SetMode(DebugMode) | ||
|
|
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
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.
| oldMode := os.Getenv("GIN_MODE") | ||
| defer os.Setenv("GIN_MODE", oldMode) | ||
| SetMode(DebugMode) |
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
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.
|
move to the next milestone |