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

Synchronize access to ctxlogrus and ctxzap fields #362

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

igorwwwwwwwwwwwwwwwwwwww

Currently the context is not safe for concurrent access. This means we cannot share the context in goroutines that could log, unless we ensure we do not add any fields later on.

The ability to add fields during the execution of a request is super valuable. This patch aims to make that possible by making ctxlogrus and ctxzap concurrency safe.

See also our downstream issue.

@codecov-io
Copy link

codecov-io commented Nov 17, 2020

Codecov Report

Merging #362 (1ebdf2c) into master (c05cb40) will decrease coverage by 0.43%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #362      +/-   ##
==========================================
- Coverage   72.81%   72.37%   -0.44%     
==========================================
  Files          41       41              
  Lines        1317     1325       +8     
==========================================
  Hits          959      959              
- Misses        304      312       +8     
  Partials       54       54              
Impacted Files Coverage Δ
logging/logrus/ctxlogrus/context.go 0.00% <0.00%> (ø)
logging/zap/ctxzap/context.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c05cb40...1ebdf2c. Read the comment docs.

@johanbrandhorst
Copy link
Collaborator

I'm not sure this is something we want to do. The existing design is already a bit messy since contexts are supposed to be immutable. The existing design relies on a value in the context being a pointer and mutable, which is very much an antipattern. If this had been implemented correctly (whereby you would get a new context every time you mutated the context) this sort of concurrent access bug would be impossible to run into. I don't think we want to encourage this pattern by making it safe to mutate concurrently. Sorry if this breaks your workflow.

Is there something else we can do to make this more clear to users?

@igorwwwwwwwwwwwwwwwwwwww
Copy link
Author

@johanbrandhorst That's fair. Returning updated immutable values is definitely cleaner.

IME it is also not always super practical though as it requires returning the context all over the place. Which is why mutable context values are quite common in practice, especially for cross-cutting concerns like logging and tracing.

If the middleware does not support concurrent access, we can probably wrap it in a lock on our end. That said, I would expect other consumers to have the same use case as us though, so it certainly may be worth considering for upstream. Even if it's a little "dirty".

@johanbrandhorst
Copy link
Collaborator

We're working on v2 at the moment and if we haven't changed this API already I'm tempted to change AddFields to return a new context rather than mutating the existing one.

@bwplotka
Copy link
Collaborator

Can we double check if this work on v2 branch? 🤗

@igorwwwwwwwwwwwwwwwwwwww
Copy link
Author

FWIW, here's how we ended up addressing this on our end: gitlab-org/gitaly!2956.

We provide our own CommandStatsMessageProducer which then extracts the (mutex-protected) values before logging them.

@devnev devnev added the v1 label Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants