synchrony / smsn

Semantic Synchrony. An experiment in cognitive and sensory augmentation.
Other
179 stars 15 forks source link

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

Closed jmatsushita closed 1 year ago

jmatsushita commented 1 year ago

In order to troubleshoot https://github.com/synchrony/smsn-mode/issues/29 I bisected commits between smsn 1.4 to develop and narrow down the problem to https://github.com/synchrony/smsn/commit/636ddb17c122da57e845ed2b831e3cdeabdb82db

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:

So 20180813 is the version that introduced the bug. The release notes point at this possible culprit https://github.com/stleary/JSON-java/issues/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.

joshsh commented 1 year ago

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 commented 1 year ago

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

jmatsushita commented 1 year ago

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?