Support empty buffers on Gzip decompression #139

erawhctim · 2023-10-31T16:10:36Z

Fixes #138

Gzipped responses that return an empty message Buffer fail with an EOFException, halting the request chain.

The fix is relatively simple, but I'm not sure if this is actually a valid state connect-kotlin should support

CLAassistant · 2023-10-31T16:10:44Z

All committers have signed the CLA.

erawhctim · 2023-10-31T22:05:37Z

okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt

+                setBody(Buffer())
+                setResponseCode(401)


This isn't quite representative of the original scenario we were seeing: the server was returning a non-empty response, but by the time it got here, the response message buffer was already empty.

Our hypothesis after stepping through the code a few times, was that parseConnectUnaryException() was consuming the message buffer before the decompress call had a change to consume it: the message was always non-empty when parseConnectUnaryException was called. We thought the root cause might be to decompress the response first, then attempt to parse it into an error message. I haven't been able to reproduce that exact scenario in this test, though; the actual root cause may still be TBD.

pkwarren · 2023-11-01T15:52:28Z

okhttp/build.gradle.kts

+    testImplementation(libs.okhttp.mockwebserver)
+    testImplementation(libs.kotlin.coroutines.test)
+    testImplementation(project(":extensions:google-java"))
+    testImplementation(project(":conformance:google-java"))


I don't think we want the okhttp module to depend on the conformance module - perhaps we just need a simple Eliza codegen in okhttp for tests so we have a service we can use.

Sure, that makes sense. Would that be set up similarly to how the :examples:generated-google-java and :examples:generated-google-javalite modules are configured?

I think depending on :examples:google-java wouldn't be too bad and that way we wouldn't have to generate those files again.

pkwarren · 2023-11-01T15:54:34Z

okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt

+
+    @Test
+    fun `compressed empty failure response is parsed correctly`() = runTest {
+        val mockWebServer = MockWebServer()


We should wire up https://github.com/square/okhttp/tree/master/mockwebserver-junit4 (or even better migrate these tests to junit jupiter and use https://github.com/square/okhttp/tree/master/mockwebserver-junit5. This will make it easier over time to add more tests here without worrying about construction/start/shutdown being called in each.

Great idea. Do you have a preference over JUnit 4 vs. 5?

My preference would be to use JUnit 5 for new tests but if that complicates things we can do a migration of all the tests in the future in a separate PR.

Ended up using JUnit4. I think it makes sense to migrate everything over to JUnit Jupiter in a future PR.

pkwarren · 2023-11-01T15:55:25Z

okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt

+        )
+
+        val request = simpleRequest {}
+        TestServiceClient(protocolClient).unaryCall(request)


Don't we want to assert something here about the returned call (that it has an appropriate connect status code)?

Yep, I can add some more detailed assertions here

erawhctim · 2023-11-03T20:55:59Z

gradle/libs.versions.toml

@@ -9,6 +9,7 @@ kotlinpoet = "1.14.2"
 mavenplugin = "0.25.3"
 moshi = "1.15.0"
 okhttp = "4.12.0"
+okhttp-junit = "5.0.0-alpha.11"


lemme know if you'd rather keep the okhttp versions the same (and depend on the older mockwebserver artifact)

Yeah let's keep them in sync.

…ptorTest

Gzip decompression fails when empty error responses are returned

pkwarren · 2023-11-08T16:09:18Z

library/src/main/kotlin/com/connectrpc/compression/GzipCompressionPool.kt

@@ -38,6 +38,8 @@ object GzipCompressionPool : CompressionPool {

    override fun decompress(buffer: Buffer): Buffer {
        val result = Buffer()
+        if (buffer.size == 0L) return result


This makes sense and is a good change (and we've amended the spec to account for it):

Servers must not attempt to decompress zero-length HTTP request content.

I think in this specific error case though, there was another issue. In

connect-kotlin/library/src/main/kotlin/com/connectrpc/protocols/ConnectInterceptor.kt

Lines 98 to 104 in 37fb4f9

val (code, exception) = if (response.code != Code.OK) {

val error = parseConnectUnaryException(code = response.code, response.headers, response.message.buffer)

error.code to error

} else {

response.code to null

}

val message = compressionPool?.decompress(response.message.buffer) ?: response.message.buffer

, we're consuming the message body (if valid json) in parseConnectUnaryException, and then on line 104 we're again trying to consume the same message body a second time. If the code is not OK, we shouldn't be attempting to consume a response message and instead should just set message to an empty buffer.

pkwarren · 2023-11-08T16:19:24Z

okhttp/src/test/kotlin/com/connectrpc/okhttp/MockWebServerTests.kt

+            assertThat(path).isEqualTo("/connectrpc.eliza.v1.ElizaService/Say")
+        }
+
+        assertThat(response.code).isEqualTo(Code.UNKNOWN)


Wrote a small connect-go example and verified that it gets the same result here. Thanks for adding these tests - they'll make it much easier to test different edge conditions outside of the conformance tests in the future.

erawhctim force-pushed the mw/gzip-decompress-EOF-error branch 2 times, most recently from 3532c77 to c6e1c36 Compare October 31, 2023 21:38

erawhctim commented Oct 31, 2023

View reviewed changes

pkwarren reviewed Nov 1, 2023

View reviewed changes

erawhctim force-pushed the mw/gzip-decompress-EOF-error branch from 67eac59 to db8ef1d Compare November 3, 2023 20:55

erawhctim commented Nov 3, 2023

View reviewed changes

erawhctim requested a review from pkwarren November 3, 2023 20:56

erawhctim added 8 commits November 7, 2023 11:34

Add failing test for GzipCompressionPool.decompress w/empty Buffer

1e1d3a6

reproduce EOFException error with new failing tests in ConnectInterce…

e785f9c

…ptorTest

Add okhttp mockwebserver to version catalog

6bca00f

Add coroutines test dependency to version catalog

91e733f

Add failing test w/MockWebServer to demonstrate issue 138

acbf39c

Gzip decompression fails when empty error responses are returned

fix EOFException on gzip decompress w/empty buffer

4969ccc

Add license header

09359de

Update MockWebServer tests to depend on generated java protos

5c620eb

erawhctim force-pushed the mw/gzip-decompress-EOF-error branch 2 times, most recently from affef99 to 5e28851 Compare November 7, 2023 21:20

erawhctim added 2 commits November 7, 2023 16:22

Add MockWebServer JUnit4 dependency & use server Rule in mock test

2c75ee6

spotless apply

ff93b51

erawhctim force-pushed the mw/gzip-decompress-EOF-error branch from f63fcfa to ff93b51 Compare November 7, 2023 21:22

pkwarren reviewed Nov 8, 2023

View reviewed changes

pkwarren approved these changes Nov 8, 2023

View reviewed changes

pkwarren merged commit 8378c54 into connectrpc:main Nov 8, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support empty buffers on Gzip decompression #139

Support empty buffers on Gzip decompression #139

erawhctim commented Oct 31, 2023

CLAassistant commented Oct 31, 2023 •

edited

Loading

erawhctim Oct 31, 2023

pkwarren Nov 1, 2023

erawhctim Nov 3, 2023

pkwarren Nov 3, 2023

pkwarren Nov 1, 2023

erawhctim Nov 3, 2023

pkwarren Nov 3, 2023

erawhctim Nov 3, 2023

pkwarren Nov 1, 2023

erawhctim Nov 3, 2023

erawhctim Nov 3, 2023

pkwarren Nov 7, 2023

pkwarren Nov 8, 2023

pkwarren Nov 8, 2023

	val (code, exception) = if (response.code != Code.OK) {
	val error = parseConnectUnaryException(code = response.code, response.headers, response.message.buffer)
	error.code to error
	} else {
	response.code to null
	}
	val message = compressionPool?.decompress(response.message.buffer) ?: response.message.buffer

Support empty buffers on Gzip decompression #139

Support empty buffers on Gzip decompression #139

Conversation

erawhctim commented Oct 31, 2023

CLAassistant commented Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Oct 31, 2023 •

edited

Loading