Synchronize access to ctxlogrus and ctxzap fields

[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]

Codecov Report

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

@@            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]

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]

@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]

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]

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

[igorwwwwwwwwwwwwwwwwwwww]

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.