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

JSON parsing fails to handle errors. #4576

Closed
3 tasks done
manchicken opened this issue Mar 24, 2023 · 11 comments
Closed
3 tasks done

JSON parsing fails to handle errors. #4576

manchicken opened this issue Mar 24, 2023 · 11 comments
Assignees
Labels
closed-for-staleness guidance General information and guidance, answers to FAQs, or recommended best practices/resources. p3 This is a minor priority issue workaround-available This issue has a work around available.

Comments

@manchicken
Copy link

Checkboxes for prior research

Describe the bug

This line should be handling the error that JSON.parse() throws whenever there is a payload returned which isn't JSON: https://github.com/aws/aws-sdk-js-v3/blob/main/clients/client-ssm/src/protocols/Aws_json1_1.ts#L20833

It would be great if this code had some sort of useful debug information that popped out as well. I'm getting this not-at-all helpful error:

SyntaxError: Unexpected token < in JSON at position 0
    at JSON.parse (<anonymous>)
    at /var/task/node_modules/@aws-sdk/client-ssm/dist-cjs/protocols/Aws_json1_1.js:15541:21
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async parseErrorBody (/var/task/node_modules/@aws-sdk/client-ssm/dist-cjs/protocols/Aws_json1_1.js:15546:19)
    at async deserializeAws_json1_1GetParameterHistoryCommandError (/var/task/node_modules/@aws-sdk/client-ssm/dist-cjs/protocols/Aws_json1_1.js:4633:15)
    at async /var/task/node_modules/@aws-sdk/middleware-serde/dist-cjs/deserializerMiddleware.js:7:24
    at async /var/task/node_modules/@aws-sdk/middleware-signing/dist-cjs/middleware.js:14:20
    at async /var/task/node_modules/@aws-sdk/middleware-retry/dist-cjs/retryMiddleware.js:27:46
    at async /var/task/node_modules/@aws-sdk/middleware-logger/dist-cjs/loggerMiddleware.js:5:22
    at async makePagedClientRequest (/var/task/node_modules/@aws-sdk/client-ssm/dist-cjs/pagination/GetParameterHistoryPaginator.js:8:12)
    at async paginateGetParameterHistory (/var/task/node_modules/@aws-sdk/client-ssm/dist-cjs/pagination/GetParameterHistoryPaginator.js:24:20)

It does eventually get to a single stack frame which covers my code, but all that does is tell me that I called your function.

JSON.parse() throws a syntax error if you don't give it valid JSON, and this code never handles that error, leaving my code to choke on an AWS response that I can't even see.

SDK version number

@aws-sdk/client-ssm version 3.299.0

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

Lambda running node 18

Reproduction Steps

I'm not at all certain how to replicate this. It seems like it most often chokes on transient API errors.

Observed Behavior

Unhelpful error message regarding JSON syntax that doesn't come from my program, and has little-to-no context on how to troubleshoot.

Expected Behavior

A useful error that helps me understand what went wrong. I'd hope that this message would at least contain:

  • Which function was called?
  • What arguments went to that function?
  • What the unexpected response was

Possible Solution

Handle the JSON.parse() exceptions, and then produce a useful error message with some useful context.

Additional Information/Context

I know that it's possible to add middleware to the client which can dump the full contents of the request and response, but this is super noisy and makes it really difficult to troubleshoot transient errors as it really increases the size of the haystack we're looking in.

@manchicken manchicken added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 24, 2023
@RanVaknin RanVaknin self-assigned this Mar 27, 2023
@RanVaknin
Copy link
Contributor

Hi @manchicken,

The error you are getting is a generic JSON parsing error, it's not something specific to the SDK, and it will not have information about the root cause of the error.
The SSM client expects a response in a JSON format so it can know how to serialize it, instead it gets a null response or a response in a different format so you receive this parse error. We need to know what the response was, so we can target the source of the problem.

know that it's possible to add middleware to the client which can dump the full contents of the request and response, but this is super noisy and makes it really difficult to troubleshoot transient errors as it really increases the size of the haystack we're looking in.

In order to root cause the actual response that was received you could use a middleware step to capture and log the raw response that caused the serialization error. That could be a transient error, or it could be a lambda timeout, or a wide variety of different errors.

If this is a transient error, there is not much the SDK team can do to help. Since the AWS SDK is a thin layer on top of a standard http client, the SDK does not have any debug capabilities when it comes to network errors, you will need to implement a proper retry strategy, and investigate your application and networking stack to mitigate those errors.

Additionally, you did not provide any of your actual code so I can only speculate on what the problem could be. Providing your actual code, or a code snippet that can reproduce the issue would be helpful.

Thanks,
Ran~

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Mar 27, 2023
@manchicken
Copy link
Author

manchicken commented Mar 27, 2023

What you said is exactly the problem: I can't tell what the actual error is because the SDK is failing to capture the actual error which appears to have been returned in some format other than JSON. This is absolutely a failure of the SDK to handle its error.

I cannot consistently reproduce the API error, so I can't give you test code. All I can do is show you where the SDK is failing to handle errors properly, thus obscuring the actual error.

It's entirely possible that the error is a rate limit or some other manner of error that would be my responsibility to manage, but since the SDK is obscuring the actual error I am unable to write code to address that root issue.

@RanVaknin RanVaknin added p1 This is a high priority issue and removed p3 This is a minor priority issue labels Mar 27, 2023
@RanVaknin RanVaknin added needs-review This issue/pr needs review from an internal developer. and removed response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. labels Mar 27, 2023
@kuhe
Copy link
Contributor

kuhe commented Mar 29, 2023

@manchicken an error thrown from the deserialization step (you will see /deserializerMiddleware.js:7:24 in the stack trace) stores the raw response in a field on the error, called error.$response. This is not visible on the object in a default console.log because of potentially sensitive information in the response headers or body.

To see the underlying error, you must intentionally log error.$response instead of just error.

I think that this is hard to discover for users, so I have submitted a PR to add a hint to the stack trace.
#4592

In this change, the hint about the $response field will appear on the second line of the error message, before the start of the first stack frame.

TypeError: stream.pipe is not a function
  To see the raw response, inspect the hidden field {error}.$response on this object.
    at ... (stack frame)

@manchicken
Copy link
Author

That won't help for this, I don't think. Doesn't they line I linked precede the middleware?

@kuhe
Copy link
Contributor

kuhe commented Mar 29, 2023

The error's stack trace includes the deserializer middleware, which means it goes through there. The $response field is added in a catch statement and the error rethrown, as shown in the diff of #4592.

In general the protocol functions are called by the serializer and deserializer middleware functions.

@kuhe kuhe added workaround-available This issue has a work around available. and removed needs-review This issue/pr needs review from an internal developer. labels Mar 29, 2023
@kuhe
Copy link
Contributor

kuhe commented Mar 29, 2023

Here is an example using the operation you were using:

const { SSMClient, GetParameterHistoryCommand } = require("@aws-sdk/client-ssm");
const { HttpResponse } = require("@aws-sdk/protocol-http");

const client = new SSMClient({
  requestHandler: new (class {
    async handle() {
      return {
        response: new HttpResponse({
          statusCode: 503,
          headers: {},
          body: Buffer.from("some unparseable response"),
        }),
      };
    }
  })(),
});

(async () => {
  const list = await client.send(new GetParameterHistoryCommand({})).catch((_) => _);

  console.log(list);
  console.log(list.$response);
})();

output:

SyntaxError: Unexpected token 's', "some unpar"... is not valid JSON
  Deserialization error: to see the raw response, inspect the hidden field {error}.$response on this object.
    at JSON.parse (<anonymous>)
    ... {
  '$metadata': { attempts: 1, totalRetryDelay: 0 }
}
HttpResponse {
  statusCode: 503,
  headers: {},
  body: <Buffer 73 6f 6d 65 20 75 6e 70 61 72 73 65 61 62 6c 65 20 72 65 73 70 6f 6e 73 65>
}

@RanVaknin RanVaknin added p3 This is a minor priority issue response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. and removed p1 This is a high priority issue labels Mar 30, 2023
@manchicken
Copy link
Author

manchicken commented Apr 1, 2023

How am I to know which http error status to trap? This seems like an awful lot of complexity you're asking me to add to my code in avoidance of your SDK trapping API errors.

The SDK should trap its own errors, not rely on the caller to inject code for error trapping. This JSON parsing error is clearly the responsibility of this SDK, which should properly handle exception cases introduced by the APIs that they call.

This may be a short-term workaround for some, but it won't work for everybody, especially when you factor in the volume of requests made by this SDK. In an application which makes a bunch of calls, this middleware hack would do more to obscure the error than to reveal it.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Apr 2, 2023
@kuhe
Copy link
Contributor

kuhe commented Apr 17, 2023

When you console.log(error.$response) the underlying reason is no longer hidden by the JSON parse error.
It is then up to you to decide what to do in response to that error.

You can, for example, retry on all 500 errors with

const result = await client.send(new GetParameterHistoryCommand({})).catch((err) => {
  console.error(err.$response);
  if (err.$response.statusCode >= 500) {
      // retry
  }
});

@manchicken
Copy link
Author

manchicken commented May 12, 2023

It seems wrong to close this even though the SDK is clearly failing to reasonably handle exceptions in JSON parsing. While work-arounds have been discussed, nobody has addressed the actual bug.

The bug has not been fixed.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2023
@RanVaknin RanVaknin reopened this Sep 6, 2023
@kuhe kuhe removed their assignment Sep 11, 2023
@RanVaknin
Copy link
Contributor

RanVaknin commented Jul 5, 2024

Hi @manchicken ,

Each AWS service adheres to a defined wire protocol (XML, JSON, CBOR, etc.), and the SDK expects responses in these formats. When a response deviates from the expected format, the SDK will throw a deserialization error as part of its standard operation. This behavior is by design, as the SDK is built to process responses strictly according to the protocol specification.

To assist with diagnosing issues where the response format is unexpected, you need to examine the raw network response before it undergoes deserialization. This can be done with the functionality @kuhe introduced that also allows you to do your own error handling, or through logging middleware or the logger interface provided on the client. This approach is consistent across all modern Smithy-based SDKs. The purpose here is not to modify the SDK to suppress or handle these errors since they are not modeled and do not conform to the protocol set by the server, but rather to provide you tools to identify and address the underlying cause—whether it is an anomaly in server behavior or a networking issue.

Since you now should be able to examine the underlying raw response, you could share it with us. If it is a malformed server response, I can upstream it and investigate why this was sent that way in the first place (this shouldn't have happened) and if its an actual server side issue we will attempt to fix it.

Thanks,
Ran~

@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. guidance General information and guidance, answers to FAQs, or recommended best practices/resources. and removed bug This issue is a bug. labels Jul 5, 2024
@RanVaknin RanVaknin added closing-soon This issue will automatically close in 4 days unless further comments are made. and removed response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. labels Aug 5, 2024
@github-actions github-actions bot added closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Aug 6, 2024
@github-actions github-actions bot closed this as completed Aug 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed-for-staleness guidance General information and guidance, answers to FAQs, or recommended best practices/resources. p3 This is a minor priority issue workaround-available This issue has a work around available.
Projects
None yet
Development

No branches or pull requests

3 participants