Skip to content

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

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
Closed
yyoshiki41 wants to merge 1 commit into from
Closed

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

yyoshiki41 wants to merge 1 commit into from

Conversation

@yyoshiki41
Copy link
Contributor

@yyoshiki41 yyoshiki41 commented Apr 16, 2023 •
edited
Loading

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

CLAassistant commented Apr 16, 2023

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.

@yyoshiki41 yyoshiki41 force-pushed the fix/api-DisableConfigDir branch from 773a454 to 2ab0a97 Compare April 16, 2023 02:34
@yyoshiki41 yyoshiki41 force-pushed the fix/api-DisableConfigDir branch from 2ab0a97 to 1b112d0 Compare April 16, 2023 02:41
@hhrutter
Copy link
Collaborator

hhrutter commented Apr 16, 2023

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
Cleanup #604
c676831
@hhrutter
Copy link
Collaborator

hhrutter commented Jul 25, 2023

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yyoshiki41 @CLAassistant @hhrutter