Merge pull request #206 from well-typed/edsko/missing-trailers

Deal with missing trailers
well-typed · Jul 26, 2024 · e991d40 · e991d40
2 parents ec1c85c + 574a6a9
commit e991d40
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 11 deletions.
diff --git a/docs/demo-client.md b/docs/demo-client.md
@@ -169,7 +169,7 @@ $ cabal run demo-client --  --core sayHelloBidiStream  \
 {message: "John Ack"}
 Disconnected. Reconnecting after 1701081μs
 Reconnecting now.
-demo-client: CallClosedWithoutTrailers
+demo-client: GrpcException {grpcError = GrpcUnknown, grpcErrorMessage = Just "Call closed without trailers", grpcErrorMetadata = []}
 ```
 
 ### Dealing with unterminated streams

diff --git a/src/Network/GRPC/Client/Session.hs b/src/Network/GRPC/Client/Session.hs
@@ -77,7 +77,15 @@ instance SupportsClientRpc rpc => IsSession (ClientSession rpc) where
         -- for example, perhaps an intermediate cache has dropped the gRPC
         -- trailers entirely. We therefore check for this case separately and
         -- throw a different error.
-        throwIO $ CallClosedWithoutTrailers
+        --
+        -- We /must/ throw a GrpcException (rather than some kind of custom one)
+        -- because the spec mandates that we synthesize a status and status
+        -- message when the peer omits them.
+        throwIO $ GrpcException {
+            grpcError         = GrpcUnknown
+          , grpcErrorMessage  = Just "Call closed without trailers"
+          , grpcErrorMetadata = []
+          }
       else
         return $ parseProperTrailers' (Proxy @rpc) trailers
 
@@ -92,7 +100,7 @@ instance SupportsClientRpc rpc => InitiateSession (ClientSession rpc) where
           -- We classify the response as Trailers-Only if the grpc-status header
           -- is present, or when the HTTP status is anything other than 200 OK
           -- (which we treat, as per the spec, as an implicit grpc-status).
-          -- The 'CallClosedWithoutTrailers' case is therefore not relevant.
+          -- The "closed without trailers" case is therefore not relevant.
           case verifyAllIf connVerifyHeaders trailersOnly of
             Left  err   -> throwIO $ CallSetupInvalidResponseHeaders err
             Right _hdrs -> return $ FlowStartNoMessages trailersOnly
@@ -160,13 +168,5 @@ data InvalidTrailers =
         invalidTrailers      :: [HTTP.Header]
       , invalidTrailersError :: String
       }
-
-    -- | The server terminated without sending trailers at all
-    --
-    -- This can also happen when a server just suddenly disappears (or the
-    -- network connection is dropped). We cannot reliably distinguish between
-    -- "we lost the connection" and "the server closed the connection but did
-    -- not send us any trailers".
-  | CallClosedWithoutTrailers
   deriving stock (Show)
   deriving anyclass (Exception)
diff --git a/test-grapesy/Test/Sanity/BrokenDeployments.hs b/test-grapesy/Test/Sanity/BrokenDeployments.hs
@@ -6,6 +6,7 @@ import Control.Concurrent
 import Control.Concurrent.Async
 import Control.Exception
 import Data.ByteString.Builder qualified as ByteString (Builder)
+import Data.Text qualified as Text
 import Network.HTTP.Types qualified as HTTP
 import Network.HTTP2.Server qualified as HTTP2
 import Network.Run.TCP qualified as NetworkRun
@@ -32,6 +33,11 @@ tests = testGroup "Test.Sanity.BrokenDeployments" [
         , testCase "nonGrpcTrailersOnly" test_nonGrpcContentTypeTrailersOnly
         , testCase "missingTrailersOnly" test_missingContentTypeTrailersOnly
       ]
+    , testGroup "Omit" [
+         testCase "status"        test_omitStatus
+       , testCase "statusMessage" test_omitStatusMessage
+       , testCase "allTrailers"   test_omitAllTrailers
+       ]
     ]
 
 {-------------------------------------------------------------------------------
@@ -99,6 +105,77 @@ test_missingContentTypeTrailersOnly = test_invalidContentType def {
       responseHeaders = [ ("grpc-status", "0") ]
     }
 
+{-------------------------------------------------------------------------------
+  Omit trailers
+-------------------------------------------------------------------------------}
+
+test_omitStatus :: Assertion
+test_omitStatus = respondWith response $ \addr -> do
+    mResp :: Either GrpcException
+                    (StreamElem NoMetadata (Proto PongMessage)) <- try $
+      Client.withConnection connParams (Client.ServerInsecure addr) $ \conn ->
+        Client.withRPC conn def (Proxy @Ping) $ \call -> do
+          Client.sendFinalInput call defMessage
+          Client.recvOutput call
+    case mResp of
+      Left err
+        | grpcError err == GrpcUnknown
+        , Just msg <- grpcErrorMessage err
+        , "grpc-status" `Text.isInfixOf` msg ->
+        return ()
+      _otherwise ->
+        assertFailure $ "Unexpected response: " ++ show mResp
+  where
+    response :: Response
+    response = def {
+          responseTrailers = [
+              ("grpc-message", "Message but no status")
+            ]
+        }
+
+test_omitStatusMessage :: Assertion
+test_omitStatusMessage = respondWith response $ \addr -> do
+    mResp :: Either GrpcException
+                    (StreamElem NoMetadata (Proto PongMessage)) <- try $
+      Client.withConnection connParams (Client.ServerInsecure addr) $ \conn ->
+        Client.withRPC conn def (Proxy @Ping) $ \call -> do
+          Client.sendFinalInput call defMessage
+          Client.recvOutput call
+    case mResp of
+      Right (NoMoreElems _) ->
+        return ()
+      _otherwise ->
+        assertFailure $ "Unexpected response: " ++ show mResp
+  where
+    response :: Response
+    response = def {
+          responseTrailers = [
+              ("grpc-status", "0")
+            ]
+        }
+
+test_omitAllTrailers :: Assertion
+test_omitAllTrailers = respondWith response $ \addr -> do
+    mResp :: Either GrpcException
+                    (StreamElem NoMetadata (Proto PongMessage)) <- try $
+      Client.withConnection connParams (Client.ServerInsecure addr) $ \conn ->
+        Client.withRPC conn def (Proxy @Ping) $ \call -> do
+          Client.sendFinalInput call defMessage
+          Client.recvOutput call
+    case mResp of
+      Left err
+        | grpcError err == GrpcUnknown
+        , Just msg <- grpcErrorMessage err
+        , "closed without trailers" `Text.isInfixOf` msg ->
+        return ()
+      _otherwise ->
+        assertFailure $ "Unexpected response: " ++ show mResp
+  where
+    response :: Response
+    response = def {
+          responseTrailers = []
+        }
+
 {-------------------------------------------------------------------------------
   Test server