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

Add panic-safe TrySubmit and TrySubmitWait with backward compatibility. #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jagerente
Copy link

@Jagerente Jagerente commented Jun 20, 2024

This PR introduces new methods TrySubmit and TrySubmitWait to provide a panic-safe way to submit tasks to the worker pool, providing clear and explicit way for users to handle the stopped state of the worker pool without breaking backward compatibility by changing existing behavior in a non-obvious for users way.

  • Tests have been added.
  • Examples and usage documentation for the new methods have been added to the README.

Note:

  • The problem covered by this changes was noted earlier in Race between Submit and Stop #76.
  • There is also suggested solution provided in fix for channel to be closed gracefully #66, however IMHO it is not good, because it alters existing behavior in a non-obvious way where tasks might not be executed as expected, but instead become no-ops. So it's better to panic than do nothing in that case.

func (p *WorkerPool) TrySubmit(task func()) error {
if p.Stopped() {
return ErrWorkerStopped
}
Copy link
Owner

Choose a reason for hiding this comment

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

Seems there is a race here: If WorkerPool is stopped after the p.Stopped check, but before the call to p.Submit. Consider using recover and checking for a write to a closed channel error, and then returning ErrWorkerStopped.

@gammazero
Copy link
Owner

gammazero commented Nov 15, 2024

I was thinking of something like this: https://github.com/gammazero/workerpool/pull/80/files, but...

Really, I want workerpool.Submit to behave like sending a task to a goroutine over a channel, which is really all it is doing but in a limiting way. If that channel was closed sending to it would panic, any I want workerpool to behave the same way.

If a caller needs to handle the panic and return an error or ignore it, then they can easily wrap the channel send or the call to workerpool.Submit from inside a function that recovers from the panic and does error handling. Since this is trivial to do, it does not need to be in this library, and the caller can handle handle the panic if they want..

select {
case <-doneCh:
break
case <-ctx.Done():
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
case <-ctx.Done():
case <- time.After(100 * time.Millisecond):

Simpler to not create a context for the timeout.

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.

2 participants