Error handling is still broken with v6.0.3

[rhegner]

Expected Behavior

When calling functions like StatObjectAsync or GetObjectAsync, the SDK should throw exceptions if connection to Minio could not be established.

Current Behavior

No exceptions are thrown in SDK v6.0.2 and v6.0.3.

This is especially bad in this example where we rely on proper exception handling to detect if an object exists:

        private static async Task<bool> Exists(IMinioClient minioClient, string bucket, string objectName)
        {
            try
            {
                var request = new StatObjectArgs()
                    .WithBucket(bucket)
                    .WithObject(objectName);
                await minioClient.StatObjectAsync(request);
                return true;
            }
            catch (ObjectNotFoundException)
            {
                return false;
            }
        }

The code returns true even though the Minio server could not even be reached.

Steps to Reproduce (for bugs)

This is my testbed:

using Microsoft.Extensions.Configuration;
using Minio;
using Minio.DataModel.Args;
using Minio.Exceptions;

namespace MinioSdkTest
{
    internal class TestException : Exception;

    internal class Program
    {
        private const string ValidMinioEndpoint = "http://localhost:9000";
        private const string InvalidMinioEndpoint = "http://localhost:9005";
        private const string Bucket = "miniotest";
        private const string ExistingObject = "Rechnung (34).pdf";
        private const string NonexistingObject = "Rechnung (xx).pdf";

        static async Task Main(string[] args)
        {
            // Minio Version                                6.0.0   6.0.1   6.0.2   6.0.3
            // --------------------------------------------------------------------------
            await TestExistsWithExistingObject();       //  ok      ok      ok      ok
            await TestExistsWithNonexistingObject();    //  ok      NOK     ok      ok
            await TestExistsFromInvalidEndpoint();      //  ok      ok      NOK     NOK
            await TestDownloadWithExistingObject();     //  ok      ok      ok      ok
            await TestDownloadWithNonexistingObject();  //  ok      ok      ok      ok
            await TestDownloadFromInvalidEndpoint();    //  ok      ok      NOK     NOK
        }

        private static async Task TestExistsWithExistingObject()
        {
            var minioClient = GetMinioClient(ValidMinioEndpoint);
            var exists = await Exists(minioClient, Bucket, ExistingObject);
            if (!exists)
                throw new TestException();
        }

        private static async Task TestExistsWithNonexistingObject()
        {
            var minioClient = GetMinioClient(ValidMinioEndpoint);
            var exists = await Exists(minioClient, Bucket, NonexistingObject);
            if (exists)
                throw new TestException();
        }

        private static async Task TestExistsFromInvalidEndpoint()
        {
            try
            {
                var minioClient = GetMinioClient(InvalidMinioEndpoint);
                var exists = await Exists(minioClient, Bucket, NonexistingObject);
                throw new TestException();  // we should never get here
            }
            catch (TestException)
            {
                throw;
            }
            catch (ConnectionException)
            {
                // this is the expected outcome (throw exception if trying to access invalid endpoint)
            }
        }

        private static async Task TestDownloadWithExistingObject()
        {
            var minioClient = GetMinioClient(ValidMinioEndpoint);
            var data = await Download(minioClient, Bucket, ExistingObject);
            if (data.Length == 0)
                throw new TestException();
        }

        private static async Task TestDownloadWithNonexistingObject()
        {
            try
            {
                var minioClient = GetMinioClient(ValidMinioEndpoint);
                var data = await Download(minioClient, Bucket, NonexistingObject);
                throw new TestException();  // we should never get here
            }
            catch (TestException)
            {
                throw;
            }
            catch (ObjectNotFoundException)
            {
                // this is the expected outcome (throw exception if trying to access nonexisting object)
            }
        }

        private static async Task TestDownloadFromInvalidEndpoint()
        {
            try
            {
                var minioClient = GetMinioClient(InvalidMinioEndpoint);
                var data = await Download(minioClient, Bucket, NonexistingObject);
                throw new TestException();  // we should never get here
            }
            catch (TestException)
            {
                throw;
            }
            catch (ConnectionException)
            {
                // this is the expected outcome (throw exception if trying to access invalid endpoint)
            }
        }

        private static async Task<bool> Exists(IMinioClient minioClient, string bucket, string objectName)
        {
            try
            {
                var request = new StatObjectArgs()
                    .WithBucket(bucket)
                    .WithObject(objectName);
                await minioClient.StatObjectAsync(request);
                return true;
            }
            catch (ObjectNotFoundException)
            {
                return false;
            }
        }

        private static async Task<Stream> Download(IMinioClient minioClient, string bucket, string objectName)
        {
            var stream = new MemoryStream();
            try
            {
                var request = new GetObjectArgs()
                    .WithBucket(bucket)
                    .WithObject(objectName)
                    .WithCallbackStream((s, ct) => s.CopyToAsync(stream, ct));
                await minioClient.GetObjectAsync(request);
                stream.Position = 0;
                return stream;
            }
            catch
            {
                stream.Dispose();
                throw;
            }
        }

        private static IMinioClient GetMinioClient(string endpoint)
        {
            var configuration = new ConfigurationBuilder().AddUserSecrets<Program>().Build();
            var accessKey = configuration["minio_access_key"];
            var secretKey = configuration["minio_secret_key"];

            var endpointUri = new Uri(endpoint);
            var minioClient = new MinioClient()
                .WithEndpoint(endpointUri.Authority)   // Authority is host:port
                .WithCredentials(accessKey, secretKey);
            if (endpointUri.Scheme == "https")
                minioClient.WithSSL();
            minioClient.Build();
            return minioClient;
        }
    }
}

Regression

Error handling is broken (in different ways) since v6.0.1.

Your Environment

Minio 6.0.3

[harshavardhana]

Moving it to relevant project

[ebozduman]

Looking into it...

[ebozduman]

Reproduced it and working on it.

[codewing]

PutObjectAsync and BucketExistsAsync also seem to ignore errors in 6.0.3 (6.0.1 is fine)

[ebozduman]

@codewing
Thank you for the info.

[ebozduman]

I think PR #1141 has fixed several issues including this one.
I appreciate if you could test and verify the fix.
Please let me know about your test results and findings.

[rhegner]

@ebozduman I did not run my tests yet, but just from looking at the code I still have some concerns regarding error handling, see #1141 (comment)

[ebozduman]

@rhegner

I agree the error handling is complex and problematic and its maintenance and testing is cumbersome.
Until it is redesigned though, we have to support it IMHO, or until mngmt makes a decision about it.

I appreciate it if you could test the fix in your environment and let us know an of course open up a new issue if required.

[tinohager]

What about the problem? Is there anything else going on here?

[ebozduman]

@tinohager

There is some work that has been done and completed to fix the issue, but testing the fix and tuning it up for all possible scenarios requires considerable amount of time.
Unfortunately, sometimes a higher priority task might slow down the progress of this issue and the reviewers are expected to be extremely thorough when the fix is ready.

[tinohager]

Maybe you should at least mark the package in Nuget/Github that there are problems with the current version.
Pin Issue

[ItsJustRuby]

I would welcome any development on this issue at all.

Just to emphasize, anyone currently starting a new project and using the most recent version of the NuGet package will get 0 error reporting and handling if they make any mistake setting up any Minio integration.

For any unfortunate future reader: I could personally fix my issue by adding .WithSSL(false) to give me:

services.AddMinio(options =>
{
    options
        .WithEndpoint(config["STORAGE_ENDPOINT"])
        .WithCredentials(config["STORAGE_ACCESS_KEY"], config["STORAGE_SECRET_KEY"])
        .WithSSL(false)
        .Build();
});

[wardboumans]

Confirming, we are running into these issues as well.

[ebozduman]

@wardboumans
Thank you for confirming it.
We are aware of it and unfortunately it took a long time to address it.
It is reevaluated and will be addressed again and available in a month frame.

[rhegner]

Unfortunately v6.0.4 does not resolve this:

// Minio Version                                6.0.0   6.0.1   6.0.2   6.0.3   6.0.4
// ----------------------------------------------------------------------------------
await TestExistsWithExistingObject();       //  ok      ok      ok      ok      ok
await TestExistsWithNonexistingObject();    //  ok      NOK     ok      ok      ok
await TestExistsFromInvalidEndpoint();      //  ok      ok      NOK     NOK     NOK
await TestDownloadWithExistingObject();     //  ok      ok      ok      ok      ok
await TestDownloadWithNonexistingObject();  //  ok      ok      ok      ok      ok
await TestDownloadFromInvalidEndpoint();    //  ok      ok      NOK     NOK     NOK
await TestWithInvalidCredentials();         //  ok      ok      NOK     NOK     NOK

We are still stuck with v6.0.0, as the error handling is completely broken with all newer versions!

[ebozduman]

@rhegner
That is unfortunately correct.
Sorry, I don't have a definitive date at this point, but it'll be addressed soon.

[tinohager]

@ebozduman At least mark the Nuget packages that they should not be used. If you don't manage to fix the error in time. The error behavior is a critical behavior from my point of view.