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

Data race fix for context slog.Logger #1193

Closed
wants to merge 5 commits into from
Closed

Conversation

wongak
Copy link

@wongak wongak commented Sep 22, 2024

See #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.

See tus#1192

This adds a mock slog Logger to create a data race issue.
Run go test -race to trigger
This fix adds two possible solutions to the data race issue.

We either add the "id" info manually when logging, or we create a
function-scoped logger based on the initial logger. The context logger
must not be mutated.
@wongak wongak changed the title Add failing test for race Data race fix for context slog.Logger Sep 22, 2024
@Acconut
Copy link
Member

Acconut commented Oct 4, 2024

Thank you for looking into this! As mentioned in #1192 (comment), 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.

@Acconut
Copy link
Member

Acconut commented Oct 4, 2024

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

@wongak
Copy link
Author

wongak commented Oct 8, 2024

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.

@Acconut
Copy link
Member

Acconut commented Oct 9, 2024

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.

@Acconut
Copy link
Member

Acconut commented Nov 12, 2024

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!

@Acconut Acconut closed this Nov 12, 2024
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 this pull request may close these issues.

2 participants