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.

Sign up for GitHub

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

Jump to bottom

handler, s3store: Fix data race problems #1199

Merged
merged 6 commits into from
Oct 4, 2024
Merged

handler, s3store: Fix data race problems #1199

merged 6 commits into from
Oct 4, 2024

Conversation

Acconut
Copy link
Member

@Acconut Acconut commented Oct 4, 2024

As discovered in #1192, Go's data race detector reports a number of issues in tusd when running its tests and as a standalone process.

In this PR, we investigate and try to address them.

@Acconut Acconut self-assigned this Oct 4, 2024
@Acconut Acconut mentioned this pull request Oct 4, 2024
Open
@Acconut
Copy link
Member Author

Acconut commented Oct 4, 2024 •
edited
Loading

First CI run finds four issues:

github.com/tus/tusd/v2/pkg/handler:

--- FAIL: TestPatch (3.05s)
    --- FAIL: TestPatch/StopUpload (1.01s)
        testing.go:1399: race detected during execution of test
    --- FAIL: TestPatch/InterruptRequestHandling (0.00s)
        testing.go:1399: race detected during execution of test

==================
WARNING: DATA RACE
Write at 0x00c000017220 by goroutine 205:
  github.com/tus/tusd/v2/pkg/handler.(*bodyReader).closeWithError()
      /home/runner/work/tusd/tusd/pkg/handler/body_reader.go:123 +0x54
  github.com/tus/tusd/v2/pkg/handler.UnroutedHandler.newContext.func1()
      /home/runner/work/tusd/tusd/pkg/handler/context.go:66 +0x152

Previous read at 0x00c000017220 by goroutine 202:
  github.com/tus/tusd/v2/pkg/handler.(*bodyReader).Read()
      /home/runner/work/tusd/tusd/pkg/handler/body_reader.go:44 +0x4d
  io.ReadAll()
      /opt/hostedtoolcache/go/1.23.2/x64/src/io/io.go:712 +0x86
  github.com/tus/tusd/v2/pkg/handler_test.readerMatcher.Matches()
      /home/runner/work/tusd/tusd/pkg/handler/utils_test.go:118 +0x7d
  github.com/tus/tusd/v2/pkg/handler_test.(*readerMatcher).Matches()
      <autogenerated>:1 +0x64
  github.com/golang/mock/gomock.(*Call).matches()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/call.go:318 +0x905
  github.com/golang/mock/gomock.callSet.FindMatch()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/callset.go:74 +0x231
  github.com/golang/mock/gomock.(*Controller).Call.func1()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/controller.go:225 +0x1b0
  github.com/golang/mock/gomock.(*Controller).Call()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/controller.go:247 +0xc6
  github.com/tus/tusd/v2/pkg/handler_test.(*MockFullUpload).WriteChunk()
      /home/runner/work/tusd/tusd/pkg/handler/handler_mock_test.go:223 +0x225
  github.com/tus/tusd/v2/pkg/handler.(*UnroutedHandler).writeChunk()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:898 +0x8ef
  github.com/tus/tusd/v2/pkg/handler.(*UnroutedHandler).PatchFile()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:794 +0x8e8
  github.com/tus/tusd/v2/pkg/handler.NewHandler.func1()
      /home/runner/work/tusd/tusd/pkg/handler/handler.go:58 +0x7aa
  net/http.HandlerFunc.ServeHTTP()
      /opt/hostedtoolcache/go/1.23.2/x64/src/net/http/server.go:2220 +0x47
  github.com/tus/tusd/v2/pkg/handler.NewHandler.(*UnroutedHandler).Middleware.func2()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:267 +0xfdb
  net/http.HandlerFunc.ServeHTTP()
      /opt/hostedtoolcache/go/1.23.2/x64/src/net/http/server.go:2220 +0x47
  github.com/tus/tusd/v2/pkg/handler.(*Handler).ServeHTTP()
      <autogenerated>:1 +0x70
  github.com/tus/tusd/v2/pkg/handler_test.(*httpTest).Run()
      /home/runner/work/tusd/tusd/pkg/handler/utils_test.go:78 +0x63e
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.func16()
      /home/runner/work/tusd/tusd/pkg/handler/patch_test.go:703 +0xf8a
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.SubTest.func35()
      /home/runner/work/tusd/tusd/pkg/handler/subtest_test.go:25 +0x125
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44

Goroutine 205 (running) created at:
  github.com/tus/tusd/v2/pkg/handler.UnroutedHandler.newContext()
      /home/runner/work/tusd/tusd/pkg/handler/context.go:60 +0x5ab
  github.com/tus/tusd/v2/pkg/handler.NewHandler.(*UnroutedHandler).Middleware.func2()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:170 +0xd7
  net/http.HandlerFunc.ServeHTTP()
      /opt/hostedtoolcache/go/1.23.2/x64/src/net/http/server.go:2220 +0x47
  github.com/tus/tusd/v2/pkg/handler.(*Handler).ServeHTTP()
      <autogenerated>:1 +0x70
  github.com/tus/tusd/v2/pkg/handler_test.(*httpTest).Run()
      /home/runner/work/tusd/tusd/pkg/handler/utils_test.go:78 +0x63e
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.func16()
      /home/runner/work/tusd/tusd/pkg/handler/patch_test.go:703 +0xf8a
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.SubTest.func35()
      /home/runner/work/tusd/tusd/pkg/handler/subtest_test.go:25 +0x125
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44

Goroutine 202 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x825
  github.com/tus/tusd/v2/pkg/handler_test.SubTest()
      /home/runner/work/tusd/tusd/pkg/handler/subtest_test.go:12 +0x808
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch()
      /home/runner/work/tusd/tusd/pkg/handler/patch_test.go:633 +0x789
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44
==================

==================
WARNING: DATA RACE
Write at 0x00c0000173a0 by goroutine 214:
  github.com/tus/tusd/v2/pkg/handler.(*bodyReader).closeWithError()
      /home/runner/work/tusd/tusd/pkg/handler/body_reader.go:123 +0x54
  github.com/tus/tusd/v2/pkg/handler.UnroutedHandler.newContext.func1()
      /home/runner/work/tusd/tusd/pkg/handler/context.go:66 +0x152

Previous read at 0x00c0000173a0 by goroutine 211:
  github.com/tus/tusd/v2/pkg/handler.(*bodyReader).Read()
      /home/runner/work/tusd/tusd/pkg/handler/body_reader.go:44 +0x4d
  io.ReadAll()
      /opt/hostedtoolcache/go/1.23.2/x64/src/io/io.go:712 +0x86
  github.com/tus/tusd/v2/pkg/handler_test.readerMatcher.Matches()
      /home/runner/work/tusd/tusd/pkg/handler/utils_test.go:118 +0x7d
  github.com/tus/tusd/v2/pkg/handler_test.(*readerMatcher).Matches()
      <autogenerated>:1 +0x64
  github.com/golang/mock/gomock.(*Call).matches()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/call.go:318 +0x905
  github.com/golang/mock/gomock.callSet.FindMatch()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/callset.go:74 +0x231
  github.com/golang/mock/gomock.(*Controller).Call.func1()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/controller.go:225 +0x1b0
  github.com/golang/mock/gomock.(*Controller).Call()
      /home/runner/go/pkg/mod/github.com/golang/mock@v1.6.0/gomock/controller.go:247 +0xc6
  github.com/tus/tusd/v2/pkg/handler_test.(*MockFullUpload).WriteChunk()
      /home/runner/work/tusd/tusd/pkg/handler/handler_mock_test.go:223 +0x225
  github.com/tus/tusd/v2/pkg/handler.(*UnroutedHandler).writeChunk()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:898 +0x8ef
  github.com/tus/tusd/v2/pkg/handler.(*UnroutedHandler).PatchFile()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:794 +0x8e8
  github.com/tus/tusd/v2/pkg/handler.NewHandler.func1()
      /home/runner/work/tusd/tusd/pkg/handler/handler.go:58 +0x7aa
  net/http.HandlerFunc.ServeHTTP()
      /opt/hostedtoolcache/go/1.23.2/x64/src/net/http/server.go:2220 +0x47
  github.com/tus/tusd/v2/pkg/handler.NewHandler.(*UnroutedHandler).Middleware.func2()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:267 +0xfdb
  net/http.HandlerFunc.ServeHTTP()
      /opt/hostedtoolcache/go/1.23.2/x64/src/net/http/server.go:2220 +0x47
  github.com/tus/tusd/v2/pkg/handler.(*Handler).ServeHTTP()
      <autogenerated>:1 +0x70
  github.com/tus/tusd/v2/pkg/handler_test.(*httpTest).Run()
      /home/runner/work/tusd/tusd/pkg/handler/utils_test.go:78 +0x63e
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.func18()
      /home/runner/work/tusd/tusd/pkg/handler/patch_test.go:810 +0xd4a
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.SubTest.func37()
      /home/runner/work/tusd/tusd/pkg/handler/subtest_test.go:25 +0x125
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44

Goroutine 214 (running) created at:
  github.com/tus/tusd/v2/pkg/handler.UnroutedHandler.newContext()
      /home/runner/work/tusd/tusd/pkg/handler/context.go:60 +0x5ab
  github.com/tus/tusd/v2/pkg/handler.NewHandler.(*UnroutedHandler).Middleware.func2()
      /home/runner/work/tusd/tusd/pkg/handler/unrouted_handler.go:170 +0xd7
  net/http.HandlerFunc.ServeHTTP()
      /opt/hostedtoolcache/go/1.23.2/x64/src/net/http/server.go:2220 +0x47
  github.com/tus/tusd/v2/pkg/handler.(*Handler).ServeHTTP()
      <autogenerated>:1 +0x70
  github.com/tus/tusd/v2/pkg/handler_test.(*httpTest).Run()
      /home/runner/work/tusd/tusd/pkg/handler/utils_test.go:78 +0x63e
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.func18()
      /home/runner/work/tusd/tusd/pkg/handler/patch_test.go:810 +0xd4a
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch.SubTest.func37()
      /home/runner/work/tusd/tusd/pkg/handler/subtest_test.go:25 +0x125
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44

Goroutine 211 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x825
  github.com/tus/tusd/v2/pkg/handler_test.SubTest()
      /home/runner/work/tusd/tusd/pkg/handler/subtest_test.go:12 +0x908
  github.com/tus/tusd/v2/pkg/handler_test.TestPatch()
      /home/runner/work/tusd/tusd/pkg/handler/patch_test.go:757 +0x889
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44
==================

github.com/tus/tusd/v2/pkg/s3store:

--- FAIL: TestConcatUploadsUsingMultipart (0.00s)
    testing.go:1399: race detected during execution of test
--- FAIL: TestWriteChunkCleansUpTempFiles (0.00s)
    testing.go:1399: race detected during execution of test

==================
WARNING: DATA RACE
Read at 0x00c000334030 by goroutine 109:
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).concatUsingMultipart.func1()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:1005 +0x8a5
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).concatUsingMultipart.gowrap1()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:1006 +0x61

Previous write at 0x00c000334030 by goroutine 108:
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).concatUsingMultipart()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:984 +0x34c
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).ConcatUploads()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:909 +0x298
  github.com/tus/tusd/v2/pkg/s3store.TestConcatUploadsUsingMultipart()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store_test.go:1310 +0x2bb5
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44

Goroutine 109 (running) created at:
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).concatUsingMultipart()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:990 +0x15c
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).ConcatUploads()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:909 +0x298
  github.com/tus/tusd/v2/pkg/s3store.TestConcatUploadsUsingMultipart()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store_test.go:1310 +0x2bb5
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44

Goroutine 108 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x825
  testing.runTests.func1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:2168 +0x85
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.runTests()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:2166 +0x8be
  testing.(*M).Run()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:2034 +0xf17
  main.main()
      _testmain.go:107 +0x164
==================

==================
WARNING: DATA RACE
Write at 0x00c000168430 by goroutine 120:
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).uploadParts.func2()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:515 +0x715
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).uploadParts.gowrap2()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:522 +0x6e

Previous write at 0x00c000168430 by goroutine 119:
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).uploadParts.func2()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:515 +0x715
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).uploadParts.gowrap2()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:522 +0x6e

Goroutine 120 (running) created at:
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).uploadParts()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:501 +0xdf5
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).WriteChunk()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:430 +0x477
  github.com/tus/tusd/v2/pkg/s3store.TestWriteChunkCleansUpTempFiles()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store_test.go:1462 +0x1232
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44

Goroutine 119 (running) created at:
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).uploadParts()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:501 +0xdf5
  github.com/tus/tusd/v2/pkg/s3store.(*s3Upload).WriteChunk()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store.go:430 +0x477
  github.com/tus/tusd/v2/pkg/s3store.TestWriteChunkCleansUpTempFiles()
      /home/runner/work/tusd/tusd/pkg/s3store/s3store_test.go:1462 +0x1232
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.2/x64/src/testing/testing.go:1743 +0x44
==================

@Acconut
Copy link
Member Author

Acconut commented Oct 4, 2024

The data race belonging to the logger setup, as mentioned in #1192 (comment) by @wongak, is not reported by the test suite. Another thing we have to look into separately.

@Acconut
Copy link
Member Author

Acconut commented Oct 4, 2024

CI is happy now and I also do not get any warnings when running go run -race cmd/tusd/main.go -hooks-http ... with manual upload tests. While this doesn't address the original data race issue from #1192, I will merge this PR and keep the other issue open to see if we can address it as well.

@Acconut Acconut changed the title Address data race issues handler, s3store: Fix data race problems Oct 4, 2024
@Acconut Acconut merged commit 8d43cf2 into main Oct 4, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Acconut Acconut

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Acconut