feat(iter): Added context accepting variants of Map & ForEach #114
Conversation
|
Hi @rkoehl05! Thanks for the PR and sorry for the slow response. Taking a look this morning |
iter/iter.go
Outdated
| runner := pool.New(). | ||
| WithContext(octx). | ||
| WithCancelOnError(). | ||
| WithFirstError(). | ||
| WithMaxGoroutines(iter.MaxGoroutines) |
There was a problem hiding this comment.
I would prefer to not add a dependency on pool in the iter package. Because the iter package knows the number of inputs and the number of outputs in advance, it can be considerably more efficient than the pool package, which must work for an unbounded number of iterations.
I think it would be good to reconcile this PR with the patterns in #104. In particular, the FailFast flag should mean similar things iterating with ForEachErr and ForEachCtx.
There was a problem hiding this comment.
The abstractions built into ContextPool offered a simple way to achieve the functionality I was looking for with regard to context cancellation without having to duplicate code. Both iter and pool used waitgroup underneath. I'm not too sure what efficiency gains are made by having two separate implementations but I'd be happy to switch it back to original setup.
Additionally, as a further argument for using ContextPool, the FailFast behavior seems like it could be easily implemented by using the new bool to optionally call WithFirstError() on the underlying pool. Right now I call that by default assuming the caller is only interested in the first error when using a context.
I could add the *Err variants as extensions to what I have here that just return an error but don't require a context.
There was a problem hiding this comment.
the FailFast behavior seems like it could be easily implemented by using the new bool to optionally call WithFirstError() on the underlying pool
Related: #118
The abstractions built into ContextPool offered a simple way to achieve the functionality I was looking for with regard to context cancellation without having to duplicate code
You might be right. The simplicity of using a Pool to back the iter package is likely more valuable than the minor efficiency gains we get from knowing the size of the set in advance (which basically boil down to allocating in advance and not needing a mutex).
Let me noodle on the design a bit and get back to ya. I'll probably open a draft that unifies this with #104 and the pool package in general, maybe just replacing the Iterator structs with configured pools.
| // MapCtx is the same as Map except it also accepts a context | ||
| // that it uses to manages the execution of tasks. | ||
| // The context is cancelled on task failure and the first error is returned. | ||
| func (m Mapper[T, R]) MapCtx(ctx context.Context, input []T, f func(context.Context, *T) (R, error)) ([]R, error) { |
There was a problem hiding this comment.
I'm interested in this as well 🙂 What do you think about using a builder for the Mapper? I would like a WithCancelOnError as well.
|
Would be nice to get this merged! 🙏 |
Added context handling to the functions in the iter package to support cancelling execution if any of the tasks fail or if the context itself is cancelled externally. As part of this change, I also re-implemented the ForEach* functions to use ContextPool from the pool package.