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 parallel creating commit status bug with tests #21911

Merged
merged 13 commits into from
Nov 30, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 23, 2022

This PR is a follow up of #21469

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second test in the PR title seems redundant.

Also, should we label this PR as skip-changelog, or is that implied in kind/testing?

tests/integration/repo_commits_test.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2022
@delvh
Copy link
Member

delvh commented Nov 24, 2022

Also, a comment why we ignore Sqlite would be good.

@delvh
Copy link
Member

delvh commented Nov 28, 2022

Is this PR alive or dead?

@lunny lunny added type/bug backport/v1.17 outdated/backport/v1.18 This PR should be backported to Gitea 1.18 labels Nov 29, 2022
@lunny lunny added this to the 1.19.0 milestone Nov 29, 2022
@lunny
Copy link
Member Author

lunny commented Nov 29, 2022

Is this PR alive or dead?

Please review again.

1 similar comment
@lunny
Copy link
Member Author

lunny commented Nov 29, 2022

Is this PR alive or dead?

Please review again.

@lunny lunny changed the title Add test for parallel creating commit status tests Fix parallel creating commit status bug with tests Nov 29, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 29, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 30, 2022
@lunny
Copy link
Member Author

lunny commented Nov 30, 2022

make L-G-T-M work

@lunny lunny merged commit b2c4870 into go-gitea:main Nov 30, 2022
@lunny lunny deleted the lunny/commit_status branch November 30, 2022 16:41
@lunny lunny added the backport/done All backports for this PR have been created label Nov 30, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 30, 2022

Some hints:

For postgresql, if the INSERT INTO encounters duplicated key and reports error, the transaction can not continue any more. So this solution is flawed for postgresql (in rare cases). For other databases (mssql, mysql, sqlite3), if the INSERT INTO fails, the transaction can still continue, so the approach could work.

(Although I use this approach a lot, I just realized that there is a problem with postgresql recently .....)

@harryzcy
Copy link
Contributor

harryzcy commented Dec 1, 2022

Does this PR introduce a DATA RACE error in the integration test?

==================
WARNING: DATA RACE
Read at 0x000008cb6420 by goroutine 239777:
  code.gitea.io/gitea/tests/integration.getTokenForLoggedInUser()
      /drone/src/tests/integration/integration_test.go:268 +0x54
  code.gitea.io/gitea/tests/integration.NewAPITestContext()
      /drone/src/tests/integration/api_helper_for_declarative_test.go:36 +0x8c
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1.1()
      /drone/src/tests/integration/repo_commits_test.go:142 +0x84
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x188
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x40

Previous write at 0x000008cb6420 by goroutine 239776:
  code.gitea.io/gitea/tests/integration.getTokenForLoggedInUser()
      /drone/src/tests/integration/integration_test.go:268 +0x70
  code.gitea.io/gitea/tests/integration.NewAPITestContext()
      /drone/src/tests/integration/api_helper_for_declarative_test.go:36 +0x8c
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1.1()
      /drone/src/tests/integration/repo_commits_test.go:142 +0x84
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1446 +0x188
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1493 +0x40

Goroutine 239777 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1493 +0x564
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1()
      /drone/src/tests/integration/repo_commits_test.go:141 +0x138
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func2()
      /drone/src/tests/integration/repo_commits_test.go:147 +0x54

Goroutine 239776 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:1493 +0x564
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func1()
      /drone/src/tests/integration/repo_commits_test.go:141 +0x138
  code.gitea.io/gitea/tests/integration.TestRepoCommitsStatusParallel.func2()
      /drone/src/tests/integration/repo_commits_test.go:147 +0x54
==================
--- FAIL: TestRepoCommitsStatusParallel (0.88s)

@lunny
Copy link
Member Author

lunny commented Dec 1, 2022

I think you are right.

 testlogger.go:77: 2022/12/01 06:47:08 ...pi/v1/repo/status.go:65:NewCommitStatus() [E] [63884dec-12] CreateCommitStatus: NewCommitStatus[repo_id: 1, user_id: 2, sha: 65f1bf27bc3bf70f64657658635e66094edbcb4d]: generate commit status index failed: pq: current transaction is aborted, commands ignored until end of transaction block
    testlogger.go:77: 2022/12/01 06:47:08 ...eb/routing/logger.go:98:func1() [I] [63884dec-12] router: completed POST /api/v1/repos/user2/repo1/statuses/65f1bf27bc3bf70f64657658635e66094edbcb4d?token=44f685f4ffd298711136bc5065ee155259e15ba8 for , 500 Internal Server Error in 181.6ms @ repo/status.go:20(repo.NewCommitStatus)
    --- FAIL: TestRepoCommitsStatusParallel/ParallelCreateStatus_2 (0.47s)
        pull_status_test.go:104: 
            	Error Trace:	/drone/src/integration_test.go:336
            	            				/drone/src/integration_test.go:165
            	            				/drone/src/pull_status_test.go:104
            	            				/drone/src/repo_commits_test.go:141
            	Error:      	Not equal: 
            	            	expected: 201
            	            	actual  : 500
            	Test:       	TestRepoCommitsStatusParallel/ParallelCreateStatus_2
            	Messages:   	Request: POST /api/v1/repos/user2/repo1/statuses/65f1bf27bc3bf70f64657658635e66094edbcb4d?token=44f685f4ffd298711136bc5065ee155259e15ba8
        pull_status_test.go:104: Response: {"message":"","url":"http://localhost:3002/api/swagger"}

@lunny
Copy link
Member Author

lunny commented Dec 2, 2022

How did you test that? I think we have enabled RACE_ENABLED in our CI, but I couldn't find the log

@harryzcy
Copy link
Contributor

harryzcy commented Dec 2, 2022

It's in my PR, https://drone.gitea.io/go-gitea/gitea/63992. I got the issue after merging from main.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 3, 2022
* giteaofficial/main:
  docs: add `Edit this page` (go-gitea#21981)
  refactor some functions to support ctx as first parameter (go-gitea#21878)
  Remove deprecated packages & staticcheck fixes (go-gitea#22012)
  Add pnpm to packages/overview (go-gitea#22008)
  Update to Alpine 3.17 (go-gitea#21904)
  Update gitea-vet to check FSFE REUSE (go-gitea#22004)
  Multiple improvements for comment edit diff (go-gitea#21990)
  Remove session in api tests (go-gitea#21984)
  Remove duplicate "Actions" label in mobile view (go-gitea#21974)
  Fix generate index failure possibility on postgres (go-gitea#21998)
  Use path not filepath in template filenames (go-gitea#21993)
  Update chroma to v2.4.0 (go-gitea#22000)
  Util type to parse ref name (go-gitea#21969)
  Skip initing LFS storage if disabled (go-gitea#21996)
  Fix parallel creating commit status bug with tests (go-gitea#21911)
  Skip initing disabled storages (go-gitea#21985)
  Fix leaving organization bug on user settings -> orgs (go-gitea#21983)
  Fix typos (go-gitea#21979)
  Normalize `AppURL` according to RFC 3986 (go-gitea#21950)
lunny added a commit that referenced this pull request Dec 13, 2022
backport #21911 
backport #21998

Co-authored-by: silverwind <me@silverwind.io>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.18 This PR should be backported to Gitea 1.18 type/bug type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants