Skip to content

always spawn a worker with the task it was supposed to complete#112

Merged
camdencheek merged 1 commit intosourcegraph:mainfrom
Link512:main
May 3, 2023
Merged

always spawn a worker with the task it was supposed to complete#112
camdencheek merged 1 commit intosourcegraph:mainfrom
Link512:main

Conversation

@Link512
Copy link
Copy Markdown
Contributor

@Link512 Link512 commented May 2, 2023

For "unlimited" pools, in the original code:

p.handle.Go(p.worker)
p.tasks <- f

If in between the call to handle.Go and sending the task to the worker, another goroutine would call pool.Go, that task would "hijack" the newly created worker, causing the original send to block. This is undesirable behavior if the pool is unlimited.

The solution was to add an initialFunc to the worker, which will be executed before the worker starts waiting for new tasks. This ensures that a worker will first complete the task it was supposed to, then complete others.

Copy link
Copy Markdown
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

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

Nice catch, and elegant solution!

Interestingly, I expected this to add an allocation for the closure, but TIL that closures do not allocate if all the captured variables are already on the heap.

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #112 (77b74b9) into main (06d3061) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #112   +/-   ##
=======================================
  Coverage   99.30%   99.30%           
=======================================
  Files          12       12           
  Lines         432      433    +1     
=======================================
+ Hits          429      430    +1     
  Misses          3        3           
Impacted Files Coverage Δ
pool/pool.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@camdencheek camdencheek merged commit 8e5ba59 into sourcegraph:main May 3, 2023
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