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

Prevent deadlock when purging idle worker during task submission #34

Merged
merged 2 commits into from
Aug 28, 2022

Conversation

alitto
Copy link
Owner

@alitto alitto commented Aug 28, 2022

  • Fix for Deadlock on zero minimum workers  #33
  • Upgrade to go 1.19
  • Extracted counter updates from main worker function to make it simpler and more generic
  • Moved worker function to a separate file
  • Added Makefile with test targets

@codecov-commenter
Copy link

codecov-commenter commented Aug 28, 2022

Codecov Report

Merging #34 (7edfde9) into master (d01340a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #34   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         4    +1     
  Lines          376       394   +18     
=========================================
+ Hits           376       394   +18     
Impacted Files Coverage Δ
pond.go 100.00% <100.00%> (ø)
worker.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@alitto alitto marked this pull request as ready for review August 28, 2022 22:01
@alitto alitto merged commit c7a74b9 into master Aug 28, 2022
assertEqual(t, 1, pool.IdleWorkers())

// Stop an idle worker right before submitting another task
pool.maybeStopIdleWorker()

Choose a reason for hiding this comment

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

Thanks for this PR and the prompt reaction!

How does this test validate issue #33, if maybeStopIdleWorker is synchronously called by the same go routine?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This test was entering a deadlock quite consistently (although not 100% of the time). maybeStopIdleWorker executes synchronously up to this point, where it sends a nil task to kill a worker goroutine asynchronously (buffered channel). In the previous version of the lib, the idle worker counter was being updated when the killed goroutine exited, so there was a small window of time during which this call would incorrectly return false, indicating there was an idle worker available.

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.

3 participants