add NewWithContext

[kaworu]

Before this patch, forwarding context cancellation to the workerpool tasks was difficult and required to spawn a goroutine to call wp.Close when the parent context was done.

This patch introduce NewWithContext allowing callers to provide a parent context from which is derived the tasks context, removing the need to manually forward context cancellation.

[rolinh]

LGTM overall but I think the doc string could be improved for better clarity.

[rolinh]

What is the problem with this? Why is it not ideal?

[kaworu]

IMO ideally Submit should return an error when the parent context is cancelled, because what would do the task do when given an already cancelled context?

To do that, Submit would have to check the context status but I didn't feel like keeping a context reference into the workerpool struct in this PR.

[rolinh]

I disagree. Submit should submit tasks for processing and not deal with the context. If the context has been canceled by calling Close(), then Submit already returns an error. In other cases, then it's the parent context that is provided by the user that has been canceled and it should be the responsibility of the caller to correctly handle this case. Notably, how to handle the context in the task func is the responsibility of the caller.

[kaworu]

I feel bad for the task to be given a cancelled context before it could do anything, but ok.

[rolinh]

If the context, that is provided by the caller, is cancelled, don't you think the caller should stop submitting tasks?

[kaworu]

Yes, I agree. If the caller stop submitting tasks as soon as the context is cancelled, then what happen on Submit doesn't matter. That's why I think this PR implementation is an acceptable behavior.

But if — for whatever reason — the caller still Submit when the context is cancelled, it would be ideal to shortcut and return an error. I think debugging would be easier if it was the case.

[chancez]

I'm not sure the constructor is the "right" place to do this. IMO, context should be passed to blocking functions that should be something you can cancel. That would be Submit, Drain, Close, etc.

• Submit is non-blocking when there's available workers, but if there are no workers, it blocks. It should be possible to cancel the submit by providing a context. (This isn't necessarily the same context that is passed to each task).
• Close and Drain both block while waiting for tasks to finish. It should be possible to add a timeout/cancellation here as well.

On the other hand, tying context to the worker pool construction ties the entire lifecycle of every task, and Close, and Drain to the initially passed context. This might make sense in some cases, but it doesn't handle the issues I've brought up.

A similar API that has to deal with this is the http.Server. It has an internal context to handle shutdowns, and then there's also a per-request context you can set that lets you timeout individual requests, and there's even per-read/write deadline options (no context, but still deals with cancellation). This relates back to us because there's a good reason context isn't something you set on the http.Server, and instead it's on: http.Server.Shutdown(context.Context). You can relate our workerpool.Close to the server.Shutdown, and the workerpool.New to the http.Server{}. The main difference is we start a goroutine in our constructor, which IMO, is an anti-pattern anyways and we shouldn't start any Goroutines, and should instead expect the caller to do it.

[chancez]

Here's a WIP draft of what I was imagining (when discussed a few weeks ago). It's not backwards compatible, and has a few issues that would need resolving, but could be a good candidate for a 2.0.

My branch: https://github.com/chancez/workerpool/tree/pr/chancez/rewrite

This closer reflects standard Go idioms/patterns I've seen. Primarily:

• blocking APIs take a context.
• constructors do not start Go routines.
• todo: Can be restarted. (once a worker pool is closed, we should be able to Run() it again IMO).

I'm trying to decide if this PR makes sense despite this. I have my complaints about the approach in this PR, but without a bigger refactor, this might be a better short-term solution.

[kaworu]

@chancez I'm all for working on a v2 with a more idiomatic API. I took a quick look at your proposition/branch and it seems promising to me!

In the meantime, since this PR is purely additive for v1 users and could be helpful, I still think we should consider merging it.

[rolinh]

I share Alex's opinion. I think that with the experience we gained using v1, we could come up with a better API for v2. In the meantime, I think the current PR improves the situation for v1 without introducing any breaking change so I'm in favor of merging it as well.

Let's create a v2 issue where we discuss the limitations of the current implementation and try to come up with a better API. Another issue I have with the current implementation is that task run results are accumulating forcing users to call Drain() periodically when a large number of tasks is submitted. I also noticed most users tend to ignore task results as returned by Drain(). We could probably come up with a better design in the regard, maybe by leveraging Go v1.20 multiple errors wrapping capabilities.

[chancez]

LGTM. It will simplify our code so that's a win.