Fixes the missing exception throw action when an api request is executed, like BucketExistsAsync, etc.

[ebozduman]

Fixes #1118
and probably #1114, #1121, #1131 and #1132, ....
There may be some more issues filed for this very same issue.
If I find more, I'll add them here.
#1120 (The PR that tried to fix the issue)

[ramondeklein]

I can't really sign off on this. Code can be simplified, but even if you do, this core method changes semantics significantly and may have serious impact on API usage. Instead of returning an error response, this throws an exception instead.

[ramondeklein]

This code will never be executed. Because responseResult is set at line 93, the previous if statement will always be true and the exception will be rethrown.

[ebozduman]

Done!

[ramondeklein]

It makes no sense to set responseResult.Exception in the previous line and now rethrow the exception. The caller of this function will never receive the responseResult.

[ebozduman]

removed!

[ramondeklein]

All code from lines 147-155 can be replaced by throw; and you would have had the same functionality as you implemented right now.

[ebozduman]

Done!

[rhegner]

@ebozduman even with your suggested changes, that entire error handling does not look very trustworthy to me.

@ramondeklein already mentioned some issues.

I didn't dig very deep in the code base, but it seems to me that the root cause of all the evil is that there is a mix of throwing exceptions and returning (unthrown) exception objects within the ResponseResult. For example, with your latest code change, in most cases exception are (re-)thrown. But if the responseResult variable is null in line 147, the exception will not be thrown, but instead returned. The fact that these two approaches are mixed, makes understanding (and maintaining) the entire error handling logic pretty difficult and fragile. [The fact that error handling is broken in various ways across multiple versions of the library is the best proof for that.]

My suggestion would be this (again, I don't really know the code base, so I might miss something):
Remove the Exception field in ResponseResult completely and transport exceptions only by throwing them. I think this would streamline the entire exception handling logic (and I think the required changes are limited to only a small part of the codebase).

---

Additional thoughts:

I could imagine that the original rationale for transporting exceptions inside of ResponseResult instead of throwing was to facilitate the idea of invoking "error handlers".
BUT: That mechanism seems to be broken anyways, see HandleIfErrorResponse method. If there is an exception, the method throws in line 386, so the handlers will never be invoked in line 391.

So, again, I think the entire error handling needs simplification and streamlining. Let all exceptions throw. If you want to keep the concept of error handlers, invoke them from within a catch block in an appropriate place, and then re-throw.

[COCPORN]

Just merge the obvious fix.

This is embarrassing.

[rhegner]

@ebozduman these are the results of my testbench:

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

Previous versions did throw a ConnectionException, with your latest code a HttpRequestException is thrown.
In our environment it would probably be ok to receive a different exception in these cases, but generally I would consider this a breaking change.

[ebozduman]

@rhegner,
Thank you very much for testing the fix.
I'll look into the regressed exceptions.
I am also running our functional tests against 3 scenarios:

• Nonexisting endpoint
• Invalid access key
• Invalid secret key
  I'll work on all the anomalies and regressions I see in these tests.

[harshavardhana]

@rhegner, Thank you very much for testing the fix. I'll look into the regressed exceptions. I am also running our functional tests against 3 scenarios:

• Nonexisting endpoint
• Invalid access key
• Invalid secret key
  I'll work on all the anomalies and regressions I see in these tests.

@ebozduman is this done?

[ebozduman]

@harshavardhana

It is incomplete!

Replaced the existing hack fix and cleaned up all the extra code.
Also made changes according to Ramon's suggestions.
But since this change touches everybody, testing took quite sometime.
There is another tune up that needs to be done for a test, but before finishing and testing the required change, I switched to the AI storage testing. So, it is still incomplete.

[jonahbohlmann]

@ebozduman
Maybe I'm misunderstanding this, but from my side this issue makes the library unstable and unusable.
We have already encountered unexpected scenarios in our applications several times because there are no clear error messages.

Is there anything the community can do to help resolve the issue in the short term? Maybe it is possible to release a beta version so we can test current fixes?