From 13a1f3b3d504030a9dee593867a31215a5d4fd18 Mon Sep 17 00:00:00 2001
From: Preston Guillory <preston@pinterest.com>
Date: Sun, 11 Jul 2021 15:30:41 -0700
Subject: [PATCH] Stay connected after unknown_method errors

The unknown_method error was added in dc53fd9d, in part to support TTwitter
protocol negotion (i.e. Finagle). However it still closes the connection after
such an error, which means Finagle clients don't work. They need to be able to
call __can__finagle__trace__v3__, get a TApplicationException, then continue
calling other methods.

The code handling the :server_error tuple is what disconnects after an error.
In general disconnecting is the only safe choice given an internal server
error. However unknown_method belongs to a class of errors where it's safe to
remain connected -- the client has made an invalid request but otherwise the
connection is still in a valid state. So this diff adds a :client_error clause
that is like :server_error except that it keeps the connection open.

Tested with a Java client using Finagle. The client was able to call
__can__finagle__trace__v3__, get the error, then make a normal request.
---
 lib/thrift/binary/framed/protocol_handler.ex | 13 +++++++++++++
 lib/thrift/generator/binary/framed/server.ex |  2 +-
 test/thrift/generator/service_test.exs       |  5 ++++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/thrift/binary/framed/protocol_handler.ex b/lib/thrift/binary/framed/protocol_handler.ex
index aad83cc..839485b 100644
--- a/lib/thrift/binary/framed/protocol_handler.ex
+++ b/lib/thrift/binary/framed/protocol_handler.ex
@@ -257,6 +257,19 @@ defmodule Thrift.Binary.Framed.ProtocolHandler do
 
         send_reply([serialized_message | serialized_response], :continue, state)
 
+      {:client_error, %TApplicationException{} = exc} ->
+        finish_span(span, result: "client_error")
+
+        serialized_message =
+          Binary.serialize(:message_begin, {:exception, sequence_id, method_name})
+
+        serialized_exception = Binary.serialize(:application_exception, exc)
+
+        response_size = IO.iodata_length(serialized_exception)
+        gauge(:response_size, response_size, state, method: method_name, result: "client_error")
+
+        send_reply([serialized_message | serialized_exception], :continue, state)
+
       {:server_error, %TApplicationException{} = exc} ->
         finish_span(span, result: "error")
 
diff --git a/lib/thrift/generator/binary/framed/server.ex b/lib/thrift/generator/binary/framed/server.ex
index e7fa893..08a789b 100644
--- a/lib/thrift/generator/binary/framed/server.ex
+++ b/lib/thrift/generator/binary/framed/server.ex
@@ -36,7 +36,7 @@ defmodule Thrift.Generator.Binary.Framed.Server do
               message: "Unknown method: #{method}"
             )
 
-          {:server_error, error}
+          {:client_error, error}
         end
       end
     end
diff --git a/test/thrift/generator/service_test.exs b/test/thrift/generator/service_test.exs
index bef7e93..87d1b17 100644
--- a/test/thrift/generator/service_test.exs
+++ b/test/thrift/generator/service_test.exs
@@ -446,7 +446,7 @@ defmodule Thrift.Generator.ServiceTest do
     {:ok, nil} = Client.do_some_work(client, "work!")
   end
 
-  thrift_test "it handles unknown method calls", ctx do
+  thrift_test "it stays connected after unknown method calls", ctx do
     {:ok, client} = WrongClient.start_link("127.0.0.1", ctx.port)
 
     # We have a client that implements different methods than the server to
@@ -458,5 +458,8 @@ defmodule Thrift.Generator.ServiceTest do
              type: :unknown_method,
              message: "Unknown method: plang"
            }
+
+    # Still connected.
+    assert {:error, {:exception, exception}} = WrongClient.plang(client)
   end
 end