Should we defer wp.Close()

[drewgonzales360]

Hi, I had question about when we need to Close(). I've added a small block to the example on the README.

		// HERE IS NEW CODE!!
		// Let's say we throw an error here and return. wp is still processing the first 500 tasks.
		if n == 500 {
			return fmt.Errorf("n == 500")
		}

Should we call defer wp.Close() immediately after we create it to avoid the workerpool continuing tasks it has in flight (unless people want the existing tasks to keep running)? Users may want to cancel all running tasks if they aren't able to submit or cancel all existing tasks if they throw an error else where. Both are valid, but I think it's worth calling out in the docs.

func main() {
	wp := workerpool.New(runtime.NumCPU())
	for i, n := 0, int64(1_000_000_000_000_000_000); n < 1_000_000_000_000_000_100; i, n = i+1, n+1 {
		n := n // https://golang.org/doc/faq#closures_and_goroutines
		id := fmt.Sprintf("task #%d", i)
		// Use Submit to submit tasks for processing. Submit blocks when no
		// worker is available to pick up the task.
		err := wp.Submit(id, func(_ context.Context) error {
			fmt.Println("isprime", n)
			if IsPrime(n) {
				fmt.Println(n, "is prime!")
			}
			return nil
		})
		// Submit fails when the pool is closed (ErrClosed) or being drained
		// (ErrDrained). Check for the error when appropriate.
		if err != nil {
			fmt.Fprintln(os.Stderr, err)
			return
		}

		// HERE IS NEW CODE!!
		// Let's say we throw an error here and return. wp is still processing the first 500 tasks.
		if n == 500 {
			return fmt.Errorf("n == 500")
		}
	}

	// Drain prevents submitting new tasks and blocks until all submitted tasks
	// complete.
	tasks, err := wp.Drain()
	if err != nil {
		fmt.Fprintln(os.Stderr, err)
		return
	}

	// Iterating over the results is useful if non-nil errors can be expected.
	for _, task := range tasks {
		// Err returns the error that the task returned after execution.
		if err := task.Err(); err != nil {
			fmt.Println("task", task, "failed:", err)
		}
	}

	// Close should be called once the worker pool is no longer necessary.
	if err := wp.Close(); err != nil {
		fmt.Fprintln(os.Stderr, err)
	}
}

[kaworu]

Hi @drewgonzales360 👋

Should we call defer wp.Close() immediately after we create it to avoid the workerpool continuing tasks it has in flight (unless people want the existing tasks to keep running)?

Yes, you can defer it and still keep the Close() call in the example, as it is safe to call Close() twice on a workerpool (although it will return ErrClosed beyond the first time). To be honest I've never encountered this use-case where the main goroutine has an error to throw, most of the time its control flow is focused on managing the workerpool and tasks. Do you have a concrete use-case example out of curiosity?

Users may want to cancel all running tasks if they aren't able to submit

In this case they should not use this module, or ensure that the workerpool capacity is at least equal to the total number of tasks they want to run. From the README:

When the limit of concurrently running workers is reached, submitting a task blocks until a worker is able to pick it up. This behavior is intentional as it prevents from accumulating tasks which could grow unbounded.

cancel all existing tasks if they throw an error elsewhere

Yes, deferring wp.Close() is a valid solution. Maybe we should add it in the example cc @rolinh