tusd Data race fix for context slog.Logger

See https://github.com/tus/tusd/issues/1192

This adds a mock slog Logger to create a data race issue. Run go test -race to trigger.

I am currently thinking about different approaches for a fix. But it's a bit complicated.

The way httpContext is implemented does not allow any mutations. It would need to be cloned, and any requests would need to be copied as well with http.Request.WithContext to contain the httpContext.

Sep 22 '24 12:09 wongak

Thank you for looking into this! As mentioned in https://github.com/tus/tusd/issues/1192#issuecomment-2393270585, it's probably easier to set the id when creating the context. We might have to move some logic around for this, but that's doable.

Oct 04 '24 09:10 Acconut

@wongak I enabled the race detector for CI in this PR and reverted your fix (for now) in an attempt to reproduce this race warning. However, neither locally nor on CI is the WithSlog test failing. Is it reporting a race warning locally for you?

Oct 04 '24 12:10 Acconut

I can't reproduce it either. Which is a bit confusing. I got the errors before applying your changes. But with your fixes, it seems to be fine.

Oct 08 '24 20:10 wongak

The data race report from your original comment mentions the use of (*httpContext).Value(), although it doesn't provide a stack trace:

Previous read at 0x00c0006ae3b0 by goroutine 398:
  github.com/tus/tusd/v2/pkg/handler.(*httpContext).Value()
      <autogenerated>:1 +0x44

I wonder if this warning appears when the context is accessed by the data store, for example.

But if you cannot reproduce this anymore, we can also close this issue since the other data races have been addressed.

Oct 09 '24 07:10 Acconut

I am closing this PR since we are not able to reproduce this data race anymore. Feel free to reopen it if the problem appears again!

Nov 12 '24 16:11 Acconut