Report context cancel cause in DoBatchWithOptions instead of ctx.Err().

[dastrobu]

What this PR does:

Report context cancel cause in DoBatchWithOptions instead of ctx.Err().

Which issue(s) this PR fixes:

Closes #576

Checklist

• Tests updated
• CHANGELOG.md updated - the order ox entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[joe-elliott]

I'm good on this change. It seems straightforward and propagates more info (if available) back to the caller. The ring is very specialized and highly critical code. Let me try to summon an expert to confirm this won't do anything unexpected.

[pstibrany]

Cannot this introduce a subtle bugs in code that already uses cancelation with cause, but calls to DoBatchWithOptions do expect standard context errors to be returned?

WDYT about adding an option to DoBatchOptions struct to enable return of context.Cause(ctx) errors instead?

[dastrobu]

Cannot this introduce a subtle bugs in code that already uses cancelation with cause, but calls to DoBatchWithOptions do expect standard context errors to be returned?

I think it is very unlikely, but definitely possible. I don't have a good idea right now why one would do that.

WDYT about adding an option to DoBatchOptions struct to enable return of context.Cause(ctx) errors instead?

It is a viable option. It adds some complexity and makes it less likely that callers of DoBatchOptions will adopt the new (preferred) error policy, as adding an extra flag is required.

If you prefer the safer but more complex option, I can adapt the PR accordingly.

[pstibrany]

Cannot this introduce a subtle bugs in code that already uses cancelation with cause, but calls to DoBatchWithOptions do expect standard context errors to be returned?

I think it is very unlikely, but definitely possible. I don't have a good idea right now why one would do that.

I can imagine this scenario very easily, with different layers having different expectations. I don't see a good way to find this in practice, other than just making the change and testing it.

WDYT about adding an option to DoBatchOptions struct to enable return of context.Cause(ctx) errors instead?

It is a viable option. It adds some complexity and makes it less likely that callers of DoBatchOptions will adopt the new (preferred) error policy, as adding an extra flag is required.

We could also make the flag to be opt-out -- enable the new behaviour by default, but provide way to disable it in case it proves troublesome. (It is a possibly breaking change after all).

I would ask @colega for his opinion too, since he introduced context checks into this code.

Dismissing my review b/c there are some obvious concerns here and I don't want this accidentally merged.

[dastrobu]

We could also make the flag to be opt-out -- enable the new behaviour by default, but provide way to disable it in case it proves troublesome.

I like the idea. It would be a good compromise between moving forward and reducing risk.
If this is consensus, I could add something like ReportContextErrInsteadOfCause to the DoBatchOptions.

[colega]

As far as I understand context cancellations with cause, this LGTM.

I think adding the option would only make it harder to reason about in the future (as we all know nobody would remove that option).

[charleskorn]

The cancellation.NewError / cancellation.NewErrorf functions might be helpful here: they're intended to be used as the cancellation cause and satisfy errors.Is(cause, context.Canceled).

[pstibrany]

Thank you!