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

Downgrade to org.json:json:20180130 fixes synchrony/smsn-mode#29 #68

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

jmatsushita
Copy link
Contributor

In order to troubleshoot synchrony/smsn-mode#29 I bisected commits between smsn 1.4 to develop and narrow down the problem to 636ddb1

I further isolated the problem to the org.json dependency upgrade to org.json:json:20220924. I then bisected the dependency versions and identified that:

  • org.json:json:20180813 has the problem
  • org.json:json:20180130 didn't have the problem

So 20180813 is the version that introduced the bug. The release notes point at this possible culprit stleary/JSON-java#678

JSONObject(Map) now throws an exception if any of a map keys are null (#405)

I imagine that some code somewhere (probably in a serializer in the Gremlin Script Engine or SessionOpProcessor or Netty) is now throwing because of null values in the map (I assume that's what map keys are null means), which indeed there are quite a lot of in that payload:

{
  "configuration"=
    {
    "version": "1.5",
    "activityLog": "data/activity.log",
    "transactionBufferSize": 100,
    "thingNamespace": "http://example.org/things/",
    "brainstream": null,
    "verbose": false,
    "services": {
      "agentIri": null,
      "broadcast": {
        "host": "255.255.255.255",
        "port": 42000,
        "graph": null,
        "interval": 5000
      },
      "osc": {
        "host": null,
        "port": 42002,
        "graph": null,
        "interval": 0
      },
      "pubSub": {
        "host": null,
        "port": 42001,
        "graph": null,
        "interval": 0
      },
      "server": null,
      "music": null
    },
    "sources": [
      {
        "name": "private",
        "code": "a",
        "location": "data/sources/private",
        "color": 16711680
      },
      {
        "name": "personal",
        "code": "s",
        "location": "data/sources/personal",
        "color": 16760832
      },
      {
        "name": "public",
        "code": "d",
        "location": "data/sources/public",
        "color": 57344
      },
      {
        "name": "universal",
        "code": "f",
        "location": "data/sources/universal",
        "color": 255
      }
    ]
  },
  "title"="[no title]"
}

This downgrade is probably a good enough fix for now since the future v2 will look quite different.

@jmatsushita jmatsushita changed the title Downgrade to org.json:json:20080701 fixes synchrony/smsn-mode#29 Downgrade to org.json:json:20180130 fixes synchrony/smsn-mode#29 Jul 12, 2023
@joshsh
Copy link
Member

joshsh commented Jul 12, 2023

Thank you, @jmatsushita. I will merge this PR after a little more discussion.

Can you verify whether upgrading to the latest org.json version (20230618) would also fix the problem? Has the bug been addressed since 20220924? Otherwise, perhaps we will blunder into the same situation in the future.

I do not see examples of null keys in the payload provided -- only of null values. However, if omitting null values would also solve the problem, then this can be done upstream.

@joshsh joshsh merged commit 00e90c9 into synchrony:develop Jul 12, 2023
@joshsh
Copy link
Member

joshsh commented Jul 12, 2023

Note: might as well merge for now, but let's discuss the two points I brought up. Thanks.

@jmatsushita
Copy link
Contributor Author

Can you verify whether upgrading to the latest org.json version (20230618) would also fix the problem? Has the bug been addressed since 20220924? Otherwise, perhaps we will blunder into the same situation in the future.

I did check that the latest version has the "bug" indeed.

I do not see examples of null keys in the payload provided -- only of null values. However, if omitting null values would also solve the problem, then this can be done upstream.

It could be that there are null keys that weren't printed in the logs. In any case you're right, ensuring that there arent null keys/values in the configuration, would be the more sustainable way to fix this.

I'm still not clear about why we get this payload {"empty":false,"mapType":"java.util.HashMap"} and not an exception. Could there also be some problematic exception handling upstream?

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.

2 participants