From 371f1c0c435030a36cd003a62be7b8012f69f30b Mon Sep 17 00:00:00 2001
From: Josh Humphries <2035234+jhump@users.noreply.github.com>
Date: Thu, 7 Dec 2023 17:06:19 -0500
Subject: [PATCH] review feedback

---
 .../connectrpc/conformance/java/ConformanceTest.kt |  2 +-
 .../conformance/javalite/ConformanceTest.kt        |  2 +-
 .../com/connectrpc/http/HTTPClientInterface.kt     | 14 +++++++++++---
 .../kotlin/com/connectrpc/impl/ProtocolClient.kt   |  4 ----
 4 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/conformance/google-java/src/test/kotlin/com/connectrpc/conformance/java/ConformanceTest.kt b/conformance/google-java/src/test/kotlin/com/connectrpc/conformance/java/ConformanceTest.kt
index da8dd503..099353ea 100644
--- a/conformance/google-java/src/test/kotlin/com/connectrpc/conformance/java/ConformanceTest.kt
+++ b/conformance/google-java/src/test/kotlin/com/connectrpc/conformance/java/ConformanceTest.kt
@@ -56,7 +56,7 @@ class ConformanceTest(
 ) : BaseConformanceTest(protocol, serverType) {
     companion object {
         private val responseHeaders = mapOf(Pair("x-grpc-test-echo-initial", listOf("test_initial_metadata_value")))
-        private val responseTrailers = mapOf(Pair("x-grpc-test-echo-trailing-bin", listOf("CgsKCwoL")), /* base64-encoded 0x0a0b0a0b0a0b */)
+        private val responseTrailers = mapOf(Pair("x-grpc-test-echo-trailing-bin", listOf("CgsKCwoL"))) // base64-encoded 0x0a0b0a0b0a0b
         private val requestHeaders = responseHeaders + responseTrailers
     }
 
diff --git a/conformance/google-javalite/src/test/kotlin/com/connectrpc/conformance/javalite/ConformanceTest.kt b/conformance/google-javalite/src/test/kotlin/com/connectrpc/conformance/javalite/ConformanceTest.kt
index e4b91fb1..21c44643 100644
--- a/conformance/google-javalite/src/test/kotlin/com/connectrpc/conformance/javalite/ConformanceTest.kt
+++ b/conformance/google-javalite/src/test/kotlin/com/connectrpc/conformance/javalite/ConformanceTest.kt
@@ -56,7 +56,7 @@ class ConformanceTest(
 ) : BaseConformanceTest(protocol, serverType) {
     companion object {
         private val responseHeaders = mapOf(Pair("x-grpc-test-echo-initial", listOf("test_initial_metadata_value")))
-        private val responseTrailers = mapOf(Pair("x-grpc-test-echo-trailing-bin", listOf("CgsKCwoL")), /* base64-encoded 0x0a0b0a0b0a0b */)
+        private val responseTrailers = mapOf(Pair("x-grpc-test-echo-trailing-bin", listOf("CgsKCwoL"))) // base64-encoded 0x0a0b0a0b0a0b
         private val requestHeaders = responseHeaders + responseTrailers
     }
 
diff --git a/library/src/main/kotlin/com/connectrpc/http/HTTPClientInterface.kt b/library/src/main/kotlin/com/connectrpc/http/HTTPClientInterface.kt
index ccad26b1..4f1e2baf 100644
--- a/library/src/main/kotlin/com/connectrpc/http/HTTPClientInterface.kt
+++ b/library/src/main/kotlin/com/connectrpc/http/HTTPClientInterface.kt
@@ -75,9 +75,17 @@ class Stream(
 
     fun receiveClose() {
         if (!isReceiveClosed.getAndSet(true)) {
-            onReceiveClose()
-            // closing receive side implicitly closes send side, too
-            isSendClosed.set(true)
+            try {
+                onReceiveClose()
+            } finally {
+                // When receive side is closed, the send side is
+                // implicitly closed as well.
+                // We don't use sendClose() because we don't want to
+                // invoke onSendClose() since that will try to actually
+                // half-close the HTTP stream, which will fail since the
+                // closing the receive side cancels the entire thing.
+                isSendClosed.set(true)
+            }
         }
     }
 
diff --git a/library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt b/library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt
index 6dcae044..6b5345a9 100644
--- a/library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt
+++ b/library/src/main/kotlin/com/connectrpc/impl/ProtocolClient.kt
@@ -216,10 +216,6 @@ class ProtocolClient(
                         )
                         channel.send(message)
                     } catch (e: Throwable) {
-                        // TODO: setting isComplete, responseTrailers, and RPC status
-                        //       here seems wrong. What would prevent the call to
-                        //       channel.send such that we don't bother getting the
-                        //       actual result/trailers from the server?
                         isComplete = true
                         try {
                             channel.close(ConnectException(Code.UNKNOWN, exception = e))