Alter HTTP requests to stop using chunked transfer encoding

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog
 
+## TBD
+
+* Alter HTTP requests to stop using chunked transfer encoding
+  [#1077](https://github.com/bugsnag/bugsnag-android/pull/1077)
+
 ## 5.5.0 (2021-01-07)
 
 This release supports initializing Bugsnag in multi processes apps. If your app uses Bugsnag in multiple processes, you should initialize Bugsnag

bugsnag-android-core/detekt-baseline.xml

@@ -20,6 +20,7 @@
     <ID>ProtectedMemberInFinalClass:EventInternal.kt$EventInternal$protected fun shouldDiscardClass(): Boolean</ID>
     <ID>ProtectedMemberInFinalClass:EventInternal.kt$EventInternal$protected fun updateSeverityInternal(severity: Severity)</ID>
     <ID>ProtectedMemberInFinalClass:PluginClient.kt$PluginClient$protected val plugins: Set&lt;Plugin&gt;</ID>
+    <ID>ReturnCount:DefaultDelivery.kt$DefaultDelivery$fun deliver( urlString: String, streamable: JsonStream.Streamable, headers: Map&lt;String, String?&gt; ): DeliveryStatus</ID>
     <ID>VariableNaming:EventInternal.kt$EventInternal$/** * @return user information associated with this Event */ internal var _user = User(null, null, null)</ID>
   </CurrentIssues>
 </SmellBaseline>

bugsnag-android-core/src/main/java/com/bugsnag/android/DefaultDelivery.kt

@@ -1,13 +1,28 @@
 package com.bugsnag.android
 
+import java.io.ByteArrayOutputStream
 import java.io.IOException
+import java.io.PrintWriter
 import java.net.HttpURLConnection
 import java.net.HttpURLConnection.HTTP_BAD_REQUEST
 import java.net.HttpURLConnection.HTTP_CLIENT_TIMEOUT
 import java.net.HttpURLConnection.HTTP_OK
 import java.net.URL
 
-internal class DefaultDelivery(private val connectivity: Connectivity?, val logger: Logger) : Delivery {
+/**
+ * Converts a [JsonStream.Streamable] into JSON, placing it in a [ByteArray]
+ */
+internal fun serializeJsonPayload(streamable: JsonStream.Streamable): ByteArray {
+    return ByteArrayOutputStream().use { baos ->
+        JsonStream(PrintWriter(baos).buffered()).use(streamable::toStream)
+        baos.toByteArray()
+    }
+}
+
+internal class DefaultDelivery(
+    private val connectivity: Connectivity?,
+    val logger: Logger
+) : Delivery {
 
     override fun deliver(payload: Session, deliveryParams: DeliveryParams): DeliveryStatus {
         val status = deliver(deliveryParams.endpoint, payload, deliveryParams.headers)
@@ -33,36 +48,58 @@ internal class DefaultDelivery(private val connectivity: Connectivity?, val logg
         var conn: HttpURLConnection? = null
 
         try {
-            val url = URL(urlString)
-            conn = url.openConnection() as HttpURLConnection
-            conn.doOutput = true
-            conn.setChunkedStreamingMode(0)
-            conn.addRequestProperty("Content-Type", "application/json")
-
-            headers.forEach { (key, value) ->
-                if (value != null) {
-                    conn.addRequestProperty(key, value)
-                }
-            }
-
-            conn.outputStream.bufferedWriter().use { writer ->
-                streamable.toStream(JsonStream(writer))
-            }
+            val json = serializeJsonPayload(streamable)
+            conn = makeRequest(URL(urlString), json, headers)
 
             // End the request, get the response code
             val responseCode = conn.responseCode
             val status = getDeliveryStatus(responseCode)
             logRequestInfo(responseCode, conn, status)
             return status
+        } catch (oom: OutOfMemoryError) {
+            // attempt to persist the payload on disk. This approach uses streams to write to a
+            // file, which takes less memory than serializing the payload into a ByteArray, and
+            // therefore has a reasonable chance of retaining the payload for future delivery.
+            logger.w("Encountered OOM delivering payload, falling back to persist on disk", oom)
+            return DeliveryStatus.UNDELIVERED
         } catch (exception: IOException) {
             logger.w("IOException encountered in request", exception)
             return DeliveryStatus.UNDELIVERED
         } catch (exception: Exception) {
             logger.w("Unexpected error delivering payload", exception)
             return DeliveryStatus.FAILURE
         } finally {
-            IOUtils.close(conn)
+            conn?.disconnect()
+        }
+    }
+
+    private fun makeRequest(
+        url: URL,
+        json: ByteArray,
+        headers: Map<String, String?>
+    ): HttpURLConnection {
+        val conn = url.openConnection() as HttpURLConnection
+        conn.doOutput = true
+
+        // avoids creating a buffer within HttpUrlConnection, see
+        // https://developer.android.com/reference/java/net/HttpURLConnection
+        conn.setFixedLengthStreamingMode(json.size)
+
+        // calculate the SHA-1 digest and add all other headers
+        computeSha1Digest(json)?.let { digest ->
+            conn.addRequestProperty(HEADER_BUGSNAG_INTEGRITY, digest)
+        }
+        headers.forEach { (key, value) ->
+            if (value != null) {
+                conn.addRequestProperty(key, value)
+            }
         }
+
+        // write the JSON payload
+        conn.outputStream.use {
+            it.write(json)
+        }
+        return conn
     }
 
     private fun logRequestInfo(code: Int, conn: HttpURLConnection, status: DeliveryStatus) {
@@ -72,9 +109,14 @@ internal class DefaultDelivery(private val connectivity: Connectivity?, val logg
                     "headers: ${conn.headerFields}"
         )
 
+        conn.inputStream.bufferedReader().use {
+            logger.d("Received request response: ${it.readText()}")
+        }
+
         if (status != DeliveryStatus.DELIVERED) {
-            val errBody = conn.errorStream.bufferedReader().readText()
-            logger.w("Request error details: $errBody")
+            conn.errorStream.bufferedReader().use {
+                logger.w("Request error details: ${it.readText()}")
+            }
         }
     }
 

bugsnag-android-core/src/main/java/com/bugsnag/android/DeliveryHeaders.kt

@@ -7,7 +7,8 @@ import java.util.Date
 
 private const val HEADER_API_PAYLOAD_VERSION = "Bugsnag-Payload-Version"
 private const val HEADER_BUGSNAG_SENT_AT = "Bugsnag-Sent-At"
-private const val HEADER_BUGSNAG_INTEGRITY = "Bugsnag-Integrity"
+private const val HEADER_CONTENT_TYPE = "Content-Type"
+internal const val HEADER_BUGSNAG_INTEGRITY = "Bugsnag-Integrity"
 internal const val HEADER_API_KEY = "Bugsnag-Api-Key"
 internal const val HEADER_INTERNAL_ERROR = "Bugsnag-Internal-Error"
 
@@ -20,8 +21,8 @@ internal fun errorApiHeaders(payload: EventPayload): Map<String, String?> {
     return mapOf(
         HEADER_API_PAYLOAD_VERSION to "4.0",
         HEADER_API_KEY to payload.apiKey,
-        HEADER_BUGSNAG_SENT_AT to DateUtils.toIso8601(Date()),
-        HEADER_BUGSNAG_INTEGRITY to computeSha1Digest(payload)
+        HEADER_CONTENT_TYPE to "application/json",
+        HEADER_BUGSNAG_SENT_AT to DateUtils.toIso8601(Date())
     )
 }
 
@@ -30,22 +31,22 @@ internal fun errorApiHeaders(payload: EventPayload): Map<String, String?> {
  *
  * @return the HTTP headers
  */
-internal fun sessionApiHeaders(apiKey: String, payload: Session): Map<String, String?> = mapOf(
+internal fun sessionApiHeaders(apiKey: String): Map<String, String?> = mapOf(
     HEADER_API_PAYLOAD_VERSION to "1.0",
     HEADER_API_KEY to apiKey,
-    HEADER_BUGSNAG_SENT_AT to DateUtils.toIso8601(Date()),
-    HEADER_BUGSNAG_INTEGRITY to computeSha1Digest(payload)
+    HEADER_CONTENT_TYPE to "application/json",
+    HEADER_BUGSNAG_SENT_AT to DateUtils.toIso8601(Date())
 )
 
-internal fun computeSha1Digest(payload: JsonStream.Streamable): String? {
+internal fun computeSha1Digest(payload: ByteArray): String? {
     runCatching {
         val shaDigest = MessageDigest.getInstance("SHA-1")
         val builder = StringBuilder("sha1 ")
 
         // Pipe the object through a no-op output stream
         DigestOutputStream(NullOutputStream(), shaDigest).use { stream ->
-            stream.bufferedWriter().use { writer ->
-                payload.toStream(JsonStream(writer))
+            stream.buffered().use { writer ->
+                writer.write(payload)
             }
             shaDigest.digest().forEach { byte ->
                 builder.append(String.format("%02x", byte))

bugsnag-android-core/src/main/java/com/bugsnag/android/IOUtils.java

@@ -26,12 +26,6 @@ static void closeQuietly(@Nullable final Closeable closeable) {
         }
     }
 
-    static void close(@Nullable final URLConnection conn) {
-        if (conn instanceof HttpURLConnection) {
-            ((HttpURLConnection) conn).disconnect();
-        }
-    }
-
     static int copy(@NonNull final Reader input,
                     @NonNull final Writer output) throws IOException {
         char[] buffer = new char[DEFAULT_BUFFER_SIZE];

bugsnag-android-core/src/main/java/com/bugsnag/android/ImmutableConfig.kt

@@ -49,8 +49,8 @@ internal data class ImmutableConfig(
         DeliveryParams(endpoints.notify, errorApiHeaders(payload))
 
     @JvmName("getSessionApiDeliveryParams")
-    internal fun getSessionApiDeliveryParams(session: Session) =
-        DeliveryParams(endpoints.sessions, sessionApiHeaders(apiKey, session))
+    internal fun getSessionApiDeliveryParams() =
+        DeliveryParams(endpoints.sessions, sessionApiHeaders(apiKey))
 }
 
 internal fun convertToImmutableConfig(

bugsnag-android-core/src/main/java/com/bugsnag/android/SessionTracker.java

@@ -296,7 +296,7 @@ void flushStoredSession(File storedFile) {
     }
 
     DeliveryStatus deliverSessionPayload(Session payload) {
-        DeliveryParams params = configuration.getSessionApiDeliveryParams(payload);
+        DeliveryParams params = configuration.getSessionApiDeliveryParams();
         Delivery delivery = configuration.getDelivery();
         return delivery.deliver(payload, params);
     }

bugsnag-android-core/src/test/java/com/bugsnag/android/DeliveryHeadersTest.kt

@@ -2,10 +2,10 @@ package com.bugsnag.android
 
 import com.bugsnag.android.BugsnagTestUtils.generateImmutableConfig
 import org.junit.Assert
-import org.junit.Assert.assertEquals
-import org.junit.Assert.assertNotEquals
-import org.junit.Assert.assertTrue
+import org.junit.Assert.*
 import org.junit.Test
+import java.io.BufferedWriter
+import java.io.StringWriter
 import java.util.Date
 
 class DeliveryHeadersTest {
@@ -15,8 +15,10 @@ class DeliveryHeadersTest {
     @Test
     fun computeSha1Digest() {
         val payload = BugsnagTestUtils.generateEventPayload(generateImmutableConfig())
-        val firstSha = requireNotNull(computeSha1Digest(payload))
-        val secondSha = requireNotNull(computeSha1Digest(payload))
+        val payload1 = serializeJsonPayload(payload)
+        val firstSha = requireNotNull(computeSha1Digest(payload1))
+        val payload2 = serializeJsonPayload(payload)
+        val secondSha = requireNotNull(computeSha1Digest(payload2))
 
         // the hash equals the expected value
         assertTrue(firstSha.matches(sha1Regex))
@@ -26,7 +28,8 @@ class DeliveryHeadersTest {
 
         // altering the streamable alters the hash
         payload.event!!.device.id = "50923"
-        val differentSha = requireNotNull(computeSha1Digest(payload))
+        val payload3 = serializeJsonPayload(payload)
+        val differentSha = requireNotNull(computeSha1Digest(payload3))
         assertNotEquals(firstSha, differentSha)
         assertTrue(differentSha.matches(sha1Regex))
     }
@@ -37,24 +40,18 @@ class DeliveryHeadersTest {
         val payload = BugsnagTestUtils.generateEventPayload(config)
         val headers = config.getErrorApiDeliveryParams(payload).headers
         assertEquals(config.apiKey, headers["Bugsnag-Api-Key"])
-        Assert.assertNotNull(headers["Bugsnag-Sent-At"])
-        Assert.assertNotNull(headers["Bugsnag-Payload-Version"])
-
-        val integrity = requireNotNull(headers["Bugsnag-Integrity"])
-        assertTrue(integrity.matches(sha1Regex))
+        assertEquals("application/json", headers["Content-Type"])
+        assertNotNull(headers["Bugsnag-Sent-At"])
+        assertNotNull(headers["Bugsnag-Payload-Version"])
     }
 
     @Test
     fun verifySessionApiHeaders() {
         val config = generateImmutableConfig()
-        val user = User("123", "hi@foo.com", "Li")
-        val session = Session("abc", Date(0), user, 1, 0, Notifier(), NoopLogger)
-        val headers = config.getSessionApiDeliveryParams(session).headers
+        val headers = config.getSessionApiDeliveryParams().headers
         assertEquals(config.apiKey, headers["Bugsnag-Api-Key"])
-        Assert.assertNotNull(headers["Bugsnag-Sent-At"])
-        Assert.assertNotNull(headers["Bugsnag-Payload-Version"])
-
-        val integrity = requireNotNull(headers["Bugsnag-Integrity"])
-        assertTrue(integrity.matches(sha1Regex))
+        assertEquals("application/json", headers["Content-Type"])
+        assertNotNull(headers["Bugsnag-Sent-At"])
+        assertNotNull(headers["Bugsnag-Payload-Version"])
     }
 }