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

Should clone record before calling .Add on it for the Go "slog" integration #2918

Closed
cideM opened this issue Oct 8, 2024 · 1 comment · Fixed by #2929
Closed

Should clone record before calling .Add on it for the Go "slog" integration #2918

cideM opened this issue Oct 8, 2024 · 1 comment · Fixed by #2929
Assignees
Labels
bug unintended behavior that has to be fixed

Comments

@cideM
Copy link

cideM commented Oct 8, 2024

I noticed that the Go integration for slog does not call rec.Clone() before adding to the record. The documentation for slog states:

Sometimes a Handler will need to modify a Record before passing it on to another Handler or backend. A Record contains a mixture of simple public fields (e.g. Time, Level, Message) and hidden fields that refer to state (such as attributes) indirectly. This means that modifying a simple copy of a Record (e.g. by calling Record.Add or Record.AddAttrs to add attributes) may have unexpected effects on the original. Before modifying a Record, use Record.Clone to create a copy that shares no state with the original, or create a new Record with NewRecord and build up its Attrs by traversing the old ones with Record.Attrs.

@github-actions github-actions bot added the needs-triage New issues that have not yet been triaged label Oct 8, 2024
@felixge felixge self-assigned this Oct 15, 2024
felixge added a commit that referenced this issue Oct 15, 2024
Fix a bug that could manifest itself when a slog Record is modified
concurrently (A) with our handler.Handle implementation (B). If A
completes before B, then it might overwrite an attribute added by A.

Add test case demonstrating the issue.

Fixes #2918
@felixge felixge added bug unintended behavior that has to be fixed and removed needs-triage New issues that have not yet been triaged labels Oct 15, 2024
@felixge
Copy link
Member

felixge commented Oct 15, 2024

Thanks for spotting and reporting this 🙇. #2929 should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintended behavior that has to be fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants