fix

Author: wxiaoguang

services/pull/pull.go

@@ -401,14 +401,14 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) {
 				continue
 			}
 
-			// create push comment first then check pull request status so the test
-			// will get a stable result
+			// create push comment before check pull request status,
+			// then when the status is mergeable, the comment is already in database, to make testing easy and stable
 			comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID, opts.IsForcePush)
 			if err == nil && comment != nil {
 				notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment)
 			}
-			// FIXME: since AddTestPullRequestTask was called in a goroutine or a queue worker, is it still necessary to
-			// call another queue to handle the PR check? Maybe we can use checkPullRequestMergeable directly here?
+			// The caller can be in a goroutine or a "push queue", "conflict check" can be time-consuming,
+			// and the concurrency should be limited, so the conflict check will be done in another queue
 			StartPullRequestCheckImmediately(ctx, pr)
 		}
 

services/repository/push.go

@@ -310,7 +310,6 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us
 	}
 
 	// only update branch can trigger pull request task because the pull request hasn't been created yet when creating a branch
-	// since pushUpdateBrach is called in a queue worker, we don't need to call it in a go routine again
 	testPullRequestOpts := pull_service.TestPullRequestOptions{
 		RepoID:      repo.ID,
 		Doer:        pusher,
@@ -320,14 +319,7 @@ func pushUpdateBranch(_ context.Context, repo *repo_model.Repository, pusher *us
 		OldCommitID: opts.OldCommitID,
 		NewCommitID: opts.NewCommitID,
 	}
-	if setting.IsInTesting {
-		// testing mode use immediate queue which runs in the same goroutine which will
-		// cause a deadlock because checkPullRequestMergeable also needs to acquire the global lock
-		go pull_service.AddTestPullRequestTask(testPullRequestOpts)
-	} else {
-		// real queue will run in a different goroutine so it is safe to call here
-		pull_service.AddTestPullRequestTask(testPullRequestOpts)
-	}
+	go pull_service.AddTestPullRequestTask(testPullRequestOpts)
 
 	if isForcePush {
 		log.Trace("Push %s is a force push", opts.NewCommitID)

tests/integration/pull_comment_test.go

@@ -62,8 +62,6 @@ func testPullCommentRebase(t *testing.T, u *url.URL, session *TestSession) {
 	doGitPushTestRepositoryFail(dstPath, "base-repo", "local-branch/rebase:test-branch/rebase")(t)
 	doGitPushTestRepository(dstPath, "--force", "base-repo", "local-branch/rebase:test-branch/rebase")(t)
 
-	// FIXME: Both the status of the pull request before pushing and the waiting are mergeable. So that
-	// if it's fast enough here, the pull request checking might be not process yet.
 	prIssue := testWaitForPullRequestStatus(t, &issues_model.Issue{Title: testPRTitle}, issues_model.PullRequestStatusMergeable)
 	comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{
 		IssueID: prIssue.ID,