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

fix(api): Protects DisableConfigDir from concurrent access using mutex #604

Closed

Conversation

yyoshiki41
Copy link
Contributor

@yyoshiki41 yyoshiki41 commented Apr 16, 2023

Thank you for your contribution!

  1. Please do not create a Pull Request without creating an issue first.

  2. Any change needs to be discussed before proceeding.

  3. Please provide enough information for PR review.

model.ConfigPath is a global variable, so data races can happen if call api.DisableConfigDir() parallely.
see more: https://go.dev/doc/articles/race_detector#Primitive_unprotected_variable

reproduction

go test -race -run DisableConfigDir ./pkg/api/...
==================
WARNING: DATA RACE
Write at 0x000102907350 by goroutine 16:
  github.com/pdfcpu/pdfcpu/pkg/api.DisableConfigDir()
      /Users/yoshiki.nakagawa/repos/src/github.com/pdfcpu/pdfcpu/pkg/api/api.go:180 +0x64
  github.com/pdfcpu/pdfcpu/pkg/api.TestDisableConfigDir_Parallel.func1()
      /Users/yoshiki.nakagawa/repos/src/github.com/pdfcpu/pdfcpu/pkg/api/api_test.go:27 +0x58

Previous write at 0x000102907350 by goroutine 6:
  github.com/pdfcpu/pdfcpu/pkg/api.DisableConfigDir()
      /Users/yoshiki.nakagawa/repos/src/github.com/pdfcpu/pdfcpu/pkg/api/api.go:180 +0x38
  github.com/pdfcpu/pdfcpu/pkg/api.TestDisableConfigDir()
      /Users/yoshiki.nakagawa/repos/src/github.com/pdfcpu/pdfcpu/pkg/api/api_test.go:12 +0x2c
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1629 +0x40

Goroutine 16 (running) created at:
  github.com/pdfcpu/pdfcpu/pkg/api.TestDisableConfigDir_Parallel()
      /Users/yoshiki.nakagawa/repos/src/github.com/pdfcpu/pdfcpu/pkg/api/api_test.go:25 +0x58
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1576 +0x188
  testing.(*T).Run.func1()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1629 +0x40

Goroutine 6 (finished) created at:
  testing.(*T).Run()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1629 +0x5e4
  testing.runTests.func1()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:2036 +0x80
  testing.tRunner()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1576 +0x188
  testing.runTests()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:2034 +0x700
  testing.(*M).Run()
      /opt/homebrew/Cellar/go/1.20.3/libexec/src/testing/testing.go:1906 +0x950
  main.main()
      _testmain.go:49 +0x300
==================
--- FAIL: TestDisableConfigDir_Parallel (0.00s)
    api_test.go:31: DisableConfigDir is passed
    testing.go:1446: race detected during execution of test
FAIL
FAIL	github.com/pdfcpu/pdfcpu/pkg/api	0.370s
ok  	github.com/pdfcpu/pdfcpu/pkg/api/test	2.794s [no tests to run]
FAIL

⚠️ This is not a perfect solution as global variables can be changed from anywhere.
Unfortunately, I haven't solution protects this settings without breaking changes.

  1. Fixes # ?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hhrutter
Copy link
Collaborator

Hello!

I wish we could have had a chance to discuss this before you open a PR.
Please tell us your detailed usecase causing the issue.
Thank you!

hhrutter added a commit that referenced this pull request Jul 25, 2023
@hhrutter
Copy link
Collaborator

This is merged in finally!
Sorry it took so long.

@hhrutter hhrutter closed this Jul 25, 2023
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.

None yet

3 participants