Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#65 per-request allocation reduction destroys this module #68

Closed
mgabeler-lee-6rs opened this issue Dec 7, 2023 · 0 comments · Fixed by #69
Closed

#65 per-request allocation reduction destroys this module #68

mgabeler-lee-6rs opened this issue Dec 7, 2023 · 0 comments · Fixed by #69

Comments

@mgabeler-lee-6rs
Copy link
Contributor

mgabeler-lee-6rs commented Dec 7, 2023

PR #65, in an attempt to reduce allocations, actually wrecks this module and makes the output insane in some cases, and introduces a humongous memory leak.

The problem is that the returned gin.HandlerFunc closes over l the logger variable. Therefore, requests stomp over each other, modifying the same context buffers inside the logger, as a nice data race for good measure, and overall resulting in the data from every request ever issued being captured, leaking memory and making a mess of the output.

logger/logger.go

Lines 107 to 113 in 7cff53e

l = l.With().
Int("status", c.Writer.Status()).
Str("method", c.Request.Method).
Str("path", path).
Str("ip", c.ClientIP()).
Dur("latency", latency).
Str("user_agent", c.Request.UserAgent()).Logger()

The tests didn't notice this because they all use a ConsoleWriter, which obscures this duplication.

I think the fix is simple: create an event at that line instead of mutating the logger. Submitted #69 with a fix for this, and a test which demonstrates the issue.

appleboy pushed a commit that referenced this issue Mar 1, 2024
* fix: don't race poking logger variable, just make events from it

fixes: #68

* fix: need a concurrent buffer to run the current logging test

* fix: correct a data race
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant