Context cancellation error sometimes swallowed by Client.listObjectsV2

[56quarters]

Hello MinIO folks!

I work on the Mimir project. We make use of a library from Thanos for access to object storage that in turn uses MinIO for S3 interactions. I've run into something surprising when using the Client.ListObjects method. I'm not sure if this is a bug or working as expected. The summary is that depending on when exactly the context.Context passed to Client.ListObjects is cancelled, sometimes the channel from ListObjects returns no ObjectInfo results and no ObjectInfo.Err error. Other times, it returns an ObjectInfo.Err error.

Consider the following code to recreate the issue. This is mean to represent a process being in the middle of making a Client.ListObjects call when the process is stopped.

func listBucketContents(ctx context.Context) ([]string, error) {
    var bucketFiles []string

    // Force the context to be cancelled to reproduce the process being stopped
    ctx, cancel := context.WithCancel(ctx)
    go func() {
        time.Sleep(5 * time.Millisecond)
        cancel()
    }()

    opts := minio.ListObjectsOptions{Prefix: "a-dir"}
    for object := range client.ListObjects(ctx, "a-dir", opts) {
        if object.Err != nil {
            return nil, object.Err
        }
    
        bucketFiles = append(bucketFiles, object.Key)
    }
    
    return bucketFiles, nil
}

Expected behavior

Assuming that listing the objects in the directory takes longer than 5ms, I would expect Client.ListObjects to send the ctx.Err() error if ctx.Done() is true/closed.

Actual behavior

Depending on when the cancellation takes place, it's possible that no ObjectInfo.Err is returned. This can be mitigated by checking for cancellation in the caller of Client.ListObjects. Indeed, this is the solution we went with in the Thanos objstore library.

What I would like

Can you confirm that not sending ctx.Err() via the results channel is on purpose or is an oversight? If this is expected behavior, could the fact that the context cancellation error is not propagated be documented as part of the API docs?

References

• Original issue opened in Mimir project
• Hacky reproduction code in Mimir
• Fix in Thanos objstore library

Thanks!

[harshavardhana]

Thanks will take a look @56quarters

[harshavardhana]

@56quarters looking at this if the context was canceled by the caller what we are expecting is that the caller is no longer interested in the objectInfoCh

which would mean that we simply return with no error, since it is not an error condition as such because it is the caller who decided to cancel the list.

does it make sense?

[harshavardhana]

We can't safely write to the channel because the caller might have decided to not read anymore which can end up blocking the call.

Think of this like doneCh a doneCh call won't return an error back from the read channel on the results.

So we can't expect the caller to be available to read the "error". The bug is actually correctly fixed in Thanos project thanos-io/objstore#62

[harshavardhana]

Please try with #1852

[56quarters]

Please try with #1852

Thanks! I'll give this a shot tomorrow.

[harshavardhana]

So I decided against it after some discussions, context errors are actually not actually the responsibility to be bubbled up via objectInfoCh and it cannot be safely done.

[56quarters]

So I decided against it after some discussions, context errors are actually not actually the responsibility to be bubbled up via objectInfoCh and it cannot be safely done.

OK, that rationale makes sense to me. Thank you for taking a look! What do you think about mentioning this in the API docs for ListObjects (and other applicable methods)?

[harshavardhana]

OK, that rationale makes sense to me. Thank you for taking a look! What do you think about mentioning this in the API docs for ListObjects (and other applicable methods)?

Actually found a way to make sure this is honored. @56quarters

[56quarters]

Looks great, thank you!