Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More Error Info #683

Merged
merged 6 commits into from
Sep 18, 2023
Merged

More Error Info #683

merged 6 commits into from
Sep 18, 2023

Conversation

JonathanHenson
Copy link
Contributor

Working on java sdk and I needed to know what the error was so I could adapt it to the exception contract of the java sdk http spi. CRT needs to be definitive on what the error was, rather than users embedding codes such as 1029 into their application codebases. So put it here.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@JonathanHenson JonathanHenson requested a review from a team September 16, 2023 01:00
*/
public enum CrtErrorInfo {
Success(0),
TLSNegotiationFailure(1029),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should code-gen these!

Waqar took a swing at doing this for aws-crt-swift a few months back. It didn't get merged for various reasons, but I'd love to take the cod-gen approach for errors in all CRTs:

Here's Waqar's code-genned enum list:
https://github.com/awslabs/aws-crt-swift/blob/fix-error-handling-2/Source/AwsCommonRuntimeKit/crt/CRTErrorGenerated.swift

Here's his code-gen script:
https://github.com/awslabs/aws-crt-swift/blob/fix-error-handling-2/Source/Script/CRTErrorGenerator.swift

We could have a CI step that confirms it's up to date (run script, fail if file changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deferring this convo to a later time. For now I can get by with the errir code being made public on the exception class

@@ -6,13 +6,14 @@
package software.amazon.awssdk.crt.http;

import software.amazon.awssdk.crt.CRT;
import software.amazon.awssdk.crt.CrtErrorInfo;

/**
* This exception will be thrown by any exceptional cases encountered within the
* JNI bindings to the AWS Common Runtime
*/
public class HttpException extends RuntimeException {
Copy link
Contributor

Choose a reason for hiding this comment

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

My real dream for aws-crt-java would be to code-gen actual exception classes (not just enums) so Java programmers could do the idiomatic thing and:

catch (TlsNegotiationFailure e) {
    ...special...
}
catch (Exception e) {
    ...everything else...
}

instead of:

catch (HttpException e) {
    switch(e.getErrorInfo()) {
        case CrtErrorInfo.TLSNegotiationFailure:
            ...special...
            break;
        default:
            ...everything else (block 1)...
            break;
    }
}
catch (Exception e) {
    ...everything else (block 2)...
}

But that would technically be a breaking change, since we'd be changing the exact exception classes that get raised 😓

Would it be worth breaking those eggs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deferring this convo to a later time. For now I can get by with the errir code being made public on the exception class

@JonathanHenson JonathanHenson merged commit 4fd525f into main Sep 18, 2023
38 checks passed
@JonathanHenson JonathanHenson deleted the better_error_info branch September 18, 2023 17:34
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