launchdarkly / js-sdk-common

Code shared between all LaunchDarkly client-side JS-based SDKs
Other
4 stars 27 forks source link

SyntaxError: Unexpected token H in JSON at position 4104 #50

Closed matewka closed 3 years ago

matewka commented 3 years ago

Describe the bug We catch a lot (100s / day) of errors with the message as stated in the title: SyntaxError: Unexpected token H in JSON at position 4104

The token and position is different every time.

The error comes from: launchdarkly-js-sdk-common/dist/ldclient-common.es.js in Object.put at line 1:24974

and it always is a result of multiple retries on stream connection. I can tell that by seeing dozens of the following console errors preceding the JSON.parse error:

LD: [warn] Error on stream connection: {"isTrusted":true}, will continue retrying every 1000 milliseconds.

To reproduce Unfortunately, I haven't been able to reproduce the problem locally yet. We receive the error in Sentry and I've provided all information I had.

Expected behavior Maybe the JSON.parse should be handled in try/catch and proxied to the consumer in a more descriptive manner. Right now I can't tell if the error is a problem in LD itself or it's caused by a wrong flag value.

Logs

SDK version We use launchdarkly-react-client-sdk version 2.22.3 which has the following dependency tree to the common SDK:

└─┬ launchdarkly-react-client-sdk@2.22.3
  └─┬ launchdarkly-js-client-sdk@2.19.2
    └── launchdarkly-js-sdk-common@3.3.2

Language version, developer tools React 16.9.0 Sentry SDK: sentry.javascript.react v6.12.0

OS/platform Any browser

eli-darkly commented 3 years ago

I think this is very unlikely to be an issue in the js-sdk-common code. I mean, we could consider logging the error differently there as you suggested, but if it is getting malformed JSON in a stream message then the problem has got to be further upstream, either in the LaunchDarkly streaming service or in some infrastructure in between. The code here does not have any influence over how stream messages are received— that is done either by the browser's built-in EventSource component, or by an EventSource polyfill if you're using one.

I think that the best way to investigate this would be to go through the support team at support.launchdarkly.com. That investigation may still include looking into JS SDK code, in which case I'll still be involved, but I think the scope is unlikely to be limited to that. For instance, even though I don't think "a wrong flag value" could cause this (no matter what the flag values are, they are encoded to JSON using the same mechanism, which cannot produce malformed JSON— that would have to involve some kind of error in transmission), if we do need to look at your flag data I have no way to do that in the context of a GitHub issue.

eli-darkly commented 3 years ago

Also, regarding the error logging, I'm not clear on what "proxied to the consumer in a more descriptive manner" would mean. If invalid stream data was received, there is not much else to say or do about it— the SDK needs to restart the stream because otherwise it can't guarantee it hasn't missed any important data.

matewka commented 3 years ago

Thank you for the response @eli-darkly .

By "proxied to the consumer in a more descriptive manner" I meant that the LD SDK could, hypothetically, handle the error and "re-throw" it to the consumer (our application) with a defined and documented signature, so our application is aware what's going on and can react accordingly. Otherwise the only way to get rid of the error on our side is to suppress it globally but that could also suppress other JSON.parse errors too eagerly if they happen.

Anyway, I suggested it before your answer, so after your explanation I understand it wouldn't be the case anymore because the error might not be coming from the flag data as I understand.

I guess I'll move on with this issue through the support team.

eli-darkly commented 3 years ago

I meant that the LD SDK could, hypothetically, handle the error and "re-throw" it to the consumer (our application) with a defined and documented signature, so our application is aware what's going on and can react accordingly

The problem is that there isn't anywhere to re-throw it to, because this is not happening inside of a method that was called by the application. The task of receiving stream data from LaunchDarkly happens in the background; at that point in the SDK code, there is no application code higher up in the call stack, and any uncaught exceptions would either fall into the void or turn into unhandled promise rejections. (Also, as a general rule we do not want to throw exceptions from the SDK into application code, because we want usage of the SDK to be very simple and transparent, so developers can add flag evaluations throughout their existing code and not worry that there could be an unexpected early exit from a code path.)

If you didn't mean literally "throw", but just provide some kind of notification mechanism— that sounds a bit like the "connection status" or "data source status" API that exists in some of the SDKs, for instance in Java, which reports any kind of problem with the SDK's connection to LD, which could be a network issue or bad data, etc. We're in the process of adding that feature to other SDKs and we may consider implementing an event-based mechanism in the JS SDK for a similar purpose, although it would probably be simpler because 1. we want to keep this SDK relatively lightweight and 2. we have much less ability to inspect or control the stream connection, due to the use of the browser's EventSource.

dsteinbach-ep commented 2 years ago

@matewka what did the support team say? We are seeing this same error but caught only in our production Sentry logs.

matewka commented 2 years ago

@dsteinbach-ep all I know about this issue is in the thread. Plus I don't work in that company anymore.