From b4ff694cdd5d5a697dea3d001e59f2785384badd Mon Sep 17 00:00:00 2001 From: Matthew Rkiouak Date: Tue, 15 Feb 2022 15:29:53 -0500 Subject: [PATCH] [ADDED] Report Encoding exceptions during unary calls * As part of the client protojure funcitonality, exception propagation of encoding issues has typically not been done, and instead the caller has been responsible on validating their input. This PR adds exception logging and propagation on an encoding issue for unary calls only BUT note this will not prevent the unary call from being sent with an empty payload. However, since this invocation with an empty payload is existing funcitonality, this commit is made to improve debuggability and exception propagation. Signed-off-by: Matthew Rkiouak --- .gitignore | 2 +- .../src/protojure/grpc/client/utils.clj | 2 +- .../internal/grpc/client/providers/http2/core.clj | 15 +++++++++------ test/project.clj | 2 +- test/test/protojure/grpc_test.clj | 13 +++++++++++++ 5 files changed, 25 insertions(+), 9 deletions(-) diff --git a/.gitignore b/.gitignore index f3c6e85..a739fe0 100644 --- a/.gitignore +++ b/.gitignore @@ -5,7 +5,7 @@ pom.xml pom.xml.asc *.jar *.class -/.lein-* +*.lein-* /.nrepl-port .hgignore .hg/ diff --git a/modules/grpc-client/src/protojure/grpc/client/utils.clj b/modules/grpc-client/src/protojure/grpc/client/utils.clj index 1e48a83..f8f930a 100644 --- a/modules/grpc-client/src/protojure/grpc/client/utils.clj +++ b/modules/grpc-client/src/protojure/grpc/client/utils.clj @@ -54,5 +54,5 @@ resolves to a decoded result when successful. Used in remote procedure calls wi | **ch** | _core.async/channel_ | A core.async channel expected to carry the response data | " [client params ch] - (-> (grpc/invoke client params) + (-> (grpc/invoke client (assoc params :unary? true)) (p/then (fn [_] (take ch))))) \ No newline at end of file diff --git a/modules/grpc-client/src/protojure/internal/grpc/client/providers/http2/core.clj b/modules/grpc-client/src/protojure/internal/grpc/client/providers/http2/core.clj index 8451f4c..ba3cc4d 100644 --- a/modules/grpc-client/src/protojure/internal/grpc/client/providers/http2/core.clj +++ b/modules/grpc-client/src/protojure/internal/grpc/client/providers/http2/core.clj @@ -22,9 +22,9 @@ [{:keys [f] :as input} codecs content-coding max-frame-size] (when (some? input) (let [input-ch (:ch input) - output-ch (async/chan 16)] - (lpm/encode f input-ch output-ch {:codecs codecs :content-coding content-coding :max-frame-size max-frame-size}) - output-ch))) + output-ch (async/chan 16) + encode-promise (lpm/encode f input-ch output-ch {:codecs codecs :content-coding content-coding :max-frame-size max-frame-size})] + [output-ch encode-promise]))) (defn- codecs-to-accept [codecs] (clojure.string/join "," (cons "identity" (keys codecs)))) @@ -142,8 +142,8 @@ (deftype Http2Provider [context uri codecs content-coding max-frame-size input-buffer-size metadata] api/Provider - (invoke [_ {:keys [input output] :as params}] - (let [input-ch (input-pipeline input codecs content-coding max-frame-size) + (invoke [_ {:keys [input output unary?] :as params}] + (let [[input-ch encode-promise] (input-pipeline input codecs content-coding max-frame-size) meta-ch (async/chan 32) output-ch (when (some? output) (async/chan (max 32 (/ input-buffer-size max-frame-size))))] @@ -155,7 +155,10 @@ (safe-close! output-ch) (async/close! meta-ch) (safe-close! input-ch) - (throw ex))))]))) + (throw ex)))) + ;;FIXME Exception handling and propagation from the encode and decode pipelines can be further improved + ;; For streaming use cases, we do not want to fail for one bad input, for unary we do + (if unary? encode-promise nil)]))) (p/then (fn [[_ status]] (log/trace "GRPC completed:" status) status) diff --git a/test/project.clj b/test/project.clj index 13276d4..4ff80fb 100644 --- a/test/project.clj +++ b/test/project.clj @@ -1,4 +1,4 @@ -(defproject io.github.protojure/test "2.0.2-SNAPSHOT" +(defproject io.github.protojure/test "2.0.7-SNAPSHOT" :description "Test harness for protojure libs" :url "http://github.com/protojure/lib" :license {:name "Apache License 2.0" diff --git a/test/test/protojure/grpc_test.clj b/test/test/protojure/grpc_test.clj index b36a01e..4b09c6c 100644 --- a/test/test/protojure/grpc_test.clj +++ b/test/test/protojure/grpc_test.clj @@ -521,6 +521,19 @@ (p/then (fn [{:keys [message] :as result}] (is (= message "Hello, World")))))))) +(deftest unary-grpc-encoding-exception-check + (testing "Check that an error in encoding a protobuf propagates out of pipeline" + (let [input (async/chan 1) + output (async/chan 16) + client (:grpc-client @test-env) + desc {:service "example.hello.Greeter" + :method "SayHello" + :input {:f new-HelloRequest :ch input} + :output {:f pb->HelloReply :ch output}}] + + (is (thrown? Exception @(-> (client.utils/send-unary-params input {:name nil}) + (p/then (fn [_] (client.utils/invoke-unary client desc output))))))))) + ;;Note there are qualitative differences between this nil & error check and the below grpc-failing-status-check test ;; this test is run against an endpoint registrered in the pedestal interceptor stack, and so exercises ;; protojure.pedestal.interceptors.grpc pedestal interceptor, where the aforementioned below test does not