Alter HTTP requests to stop using chunked transfer encoding

[fractalwrench]

Goal

Alters HTTP requests so that they no longer use chunked transfer encoding, and instead build the request body up front. This fixes two problems:

• Proxies such as Privoxy do not support chunked transfer encoding so HTTP requests made by the notifier may always fail on certain versions of Android
• Creating a buffer that contains the entire request body allows for a SHA-1 digest to be calculated after the payload has been serialized. This avoids the possibility of SHA-1 mismatches caused by payload mutation that could result in some payloads being dropped

Changeset

• Moved Content-Type header to be included in error/session API DeliveryParams object for consistency with other headers
• Serialized error/session payload to JSON in a ByteArray, and generated the SHA-1 digest from this ByteArray which circumvents any possible change in the payload
• Wrote the JSON in the ByteArray to the HTTP request directly, rather than streaming it with chunked transfer encoding
• Called setFixedLengthStreamingMode to avoid creating an unnecessary buffer within HttpUrlConnection. Testing with unusual unicode characters revealed that this needed to be a ByteArray rather than a String to get the correct body length
• Read and closed the connection's inputstream to fix an Android platform bug that logs a warning message about connections being leaked, and to log additional information about the request
• Caught OutOfMemoryError and attempted to persist the payload to disk, as this operation takes a smaller amount of memory and is likely to succeed if memory is constrained for large payloads

Testing

Relied on existing unit and E2E test coverage.

Performance

For the default payload sent by the example app (around 17Kb), there is no discernible difference in performance for this changeset. Both v5.5.0 and the current changeset use around 0.2Mb to deliver one payload.

Datadog has confirmed that the average payload size received by the pipeline is 16-24Kb, so for the majority of use cases this change will have no performance impact.

Additional load testing

To perform stress testing, v5.5.0 and the current changeset were load tested using the following snippet which generates a 900Kb payload containing a very large amount of metadata:

repeat(1000) { a ->
    repeat(10) { z ->
        Bugsnag.addMetadata(
            "custom",
            "a long long long long long long key: $a $z",
            "a long long long long long long value: $a $z"
        )
    }
}

The additional JVM memory required to deliver one payload was as follows:

Before changeset: 1.3Mb memory usage
With changeset: 2.9Mb memory usage

Further stress testing generated 20 of these payloads simultaneously on 5 different threads (an unrealistic scenario):

Before changeset: 12.5Mb memory usage
With changeset: 18.2Mb memory usage

The majority of allocations for both changesets were for java/lang/String and java/util/concurrent/ConcurrentHashMap$Node, so it seems that JSON serialization is the dominating factor for memory usage rather than buffering the payload. It’s worth noting that manually triggering GC freed up short-lived references created during JSON serialization, so the JVM would trigger GC calls if the app was running low on memory when attempting to allocate a buffer.

It's also worth noting that the example app's payload size is around 17Kb, which is effectively the same size as the default buffer (16kb) in the previous implementation that uses streams. This suggests that for regular use without large amounts of metadata there should be no performance impact.

[bugsnagbot]

Android notifier sizes

Format	Size impact of Bugsnag (kB)	Size impact of Bugsnag when Minified (kB)
APK	1459.74	1374.4
arm64_v8a	377.51	291.5
armeabi	357.03	266.91
armeabi_v7a	340.65	250.54
x86	414.36	328.34
x86_64	397.98	311.97

Generated by 🚫 Danger

[fractalwrench]

@tomlongridge @twometresteve I've tagged for a preliminary review - I'm planning on doing some more detailed performance testing on the changeset, but it'd be good to resolve any code changes before that gets too far along.

[tomlongridge]

Changes LGTM - we just need to conclude discussions based on performance.

[twometresteve]

Also LGTM.

Scenarios ring fenced as being "apparently flaky" need to be reviewed quickly after this change, or possibly even as part of it. I already have a ticket to do that myself, so just a heads-up.

Edit: The struck-through point has now been implemented in #1081.

[twometresteve]

See 1 comment above.

[fractalwrench]

Updated PR description with details of perf testing - this is now ready for full review.

[twometresteve]

LGTM