Skip to content

Throw ctx cancelled error in thread.Parallelize#2089

Merged
unmultimedio merged 2 commits intomainfrom
jfigueroa/thread-parallelize-ctx-cancelation
May 16, 2023
Merged

Throw ctx cancelled error in thread.Parallelize#2089
unmultimedio merged 2 commits intomainfrom
jfigueroa/thread-parallelize-ctx-cancelation

Conversation

@unmultimedio
Copy link
Member

@unmultimedio unmultimedio commented May 15, 2023

Found a panic in a test, caused because of a DX assumption. From users perspective, doing:

foo := make([]Foo, someLen)
// jobs preparation... each job fills one item in []foo
if err := thread.Parallelize(
  ctx,
  jobs,
  thread.ParallelizeWithCancel(cancelGetBlobs), // if one job fails, stop all jobs
); err != nil {
  return nil, err
}
// at this point the user could assume that if no errors happened, then []foo was successfully
// populated... well, if the ctx was cancelled, some/all items in []foo might be empty/nil.
// Even if the ctx was cancelled not by an error trying to fill []foo, but upstream from
// another caller.

I will test in a dependent repo before making this ready for review. Tested.

@unmultimedio unmultimedio self-assigned this May 15, 2023
@unmultimedio unmultimedio marked this pull request as ready for review May 15, 2023 22:43
Copy link
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

This is a good find! Stamping, but may want input from others also :)

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

I'd probably delete the jobIdx stuff, see comments

select {
case <-ctx.Done():
stop = true
retErr = multierr.Append(retErr, fmt.Errorf("jobs[%d]: context canceled", jobIdx))
Copy link
Member

Choose a reason for hiding this comment

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

This should likely be ctx.Err() instead of a custom error, some people parse the specific context canceled function

select {
case <-ctx.Done():
stop = true
retErr = multierr.Append(retErr, fmt.Errorf("jobs[%d]: context canceled", jobIdx))
Copy link
Member

Choose a reason for hiding this comment

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

same comment

if err := job(ctx); err != nil {
lock.Lock()
retErr = multierr.Append(retErr, err)
retErr = multierr.Append(retErr, fmt.Errorf("jobs[%d]: %w", jobIdx, err))
Copy link
Member

Choose a reason for hiding this comment

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

I can see it both ways, I might lean to not doing a custom error at all - I know you're using %w so it can be unrolled, but no need to expose to the user that this is parallel job at all IMO

@unmultimedio unmultimedio merged commit f9c3601 into main May 16, 2023
@unmultimedio unmultimedio deleted the jfigueroa/thread-parallelize-ctx-cancelation branch May 16, 2023 15:44
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