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

Only attempt to flush queue if the underlying worker pool is not finished #18593

Merged
merged 4 commits into from
Feb 5, 2022

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Feb 4, 2022

There is a possible race whereby a worker pool could be cancelled but yet the
underlying queue is not empty. This will lead to flush-all cycling because it
cannot empty the pool.

The solution here is to add another function to the managed pools which will allow testing if the managed pool has been finished before attempting to flush.

Signed-off-by: Andrew Thornton [email protected]

…shed

There is a possible race whereby a worker pool could be cancelled but yet the
underlying queue is not empty. This will lead to flush-all cycling because it
cannot empty the pool.

Signed-off-by: Andrew Thornton <[email protected]>
zeripath added a commit to zeripath/gitea that referenced this pull request Feb 5, 2022
…shed (go-gitea#18593)

Backport go-gitea#18593

There is a possible race whereby a worker pool could be cancelled but yet the
underlying queue is not empty. This will lead to flush-all cycling because it
cannot empty the pool.

* On shutdown of Persistant Channel Queues close datachan and empty

Partial Backport go-gitea#18415

Although we attempt to empty the datachan in queues - due to
races we are better off just closing the channel and forcibly emptying
it in shutdown.

Fix go-gitea#18618

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added the backport/done All backports for this PR have been created label Feb 5, 2022
modules/queue/manager.go Outdated Show resolved Hide resolved
modules/queue/manager.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 Feb 5, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@a51d211). Click here to learn what that means.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18593   +/-   ##
=======================================
  Coverage        ?   46.29%           
=======================================
  Files           ?      846           
  Lines           ?   121201           
  Branches        ?        0           
=======================================
  Hits            ?    56106           
  Misses          ?    58288           
  Partials        ?     6807           
Impacted Files Coverage Δ
modules/queue/manager.go 40.41% <0.00%> (ø)
modules/queue/workerpool.go 53.07% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a51d211...b53f4bf. Read the comment docs.

@zeripath zeripath merged commit 7ba1b71 into go-gitea:main Feb 5, 2022
@zeripath zeripath deleted the prevent-flush-loop branch February 5, 2022 20:51
lunny pushed a commit that referenced this pull request Feb 6, 2022
…shed (#18593) (#18620)

* Only attempt to flush queue if the underlying worker pool is not finished (#18593)

Backport #18593

There is a possible race whereby a worker pool could be cancelled but yet the
underlying queue is not empty. This will lead to flush-all cycling because it
cannot empty the pool.

* On shutdown of Persistant Channel Queues close datachan and empty

Partial Backport #18415

Although we attempt to empty the datachan in queues - due to
races we are better off just closing the channel and forcibly emptying
it in shutdown.

Fix #18618

Signed-off-by: Andrew Thornton <[email protected]>

* Move zero workers warning to debug

Fix #18617

Signed-off-by: Andrew Thornton <[email protected]>

* Update modules/queue/manager.go

Co-authored-by: Gusted <[email protected]>

* Update modules/queue/manager.go

Co-authored-by: Gusted <[email protected]>

Co-authored-by: Gusted <[email protected]>
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
…shed (go-gitea#18593)

* Only attempt to flush queue if the underlying worker pool is not finished

There is a possible race whereby a worker pool could be cancelled but yet the
underlying queue is not empty. This will lead to flush-all cycling because it
cannot empty the pool.

Signed-off-by: Andrew Thornton <[email protected]>

* Apply suggestions from code review

Co-authored-by: Gusted <[email protected]>

Co-authored-by: Gusted <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants