Skip to content

Commit 9a2815a

Browse files
sundbrysundbry
committed
New test case: RPC handler when lazy response data throws an error
This reproduces a bug where an exception raised in a lazy sequence attached to a protobuf response causes the request handler to crash and time out. Signed-off-by: Ryan Sundberg <ryan@arctype.co>
1 parent 7dd9028 commit 9a2815a

5 files changed

Lines changed: 149 additions & 15 deletions

File tree

‎test/test/protojure/grpc_test.clj‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,20 @@
183183
{:body {:msg (str "Hello, " auth)}}))
184184

185185
(ShouldThrow
186-
[_ request]
187-
(throw (ex-info "This is supposed to fail" {})))
186+
[_ {{test-case :case} :grpc-params}]
187+
{:body
188+
(case test-case
189+
0 (throw (ex-info "This is supposed to fail" {}))
190+
; Non-lazy seq
191+
1 {:numbers
192+
(mapv (fn [_]
193+
(throw (ex-info "This is also supposed to fail (1)" {})))
194+
(range 3))}
195+
; Lazy seq
196+
2 {:numbers
197+
(map (fn [n]
198+
(throw (ex-info "This is also supposed to fail (2)" {})))
199+
(range 3))})})
188200

189201
(Async
190202
[_ request]
@@ -262,6 +274,16 @@
262274
[code & body]
263275
`(-check-throw ~code #(do ~@body)))
264276

277+
(defn is-grpc-error? [grpc-status response-promise]
278+
"Asserts that response promise returns a specific gRPC error code, and
279+
returns the exception."
280+
(let [ex (is (thrown? Exception @response-promise))
281+
cause (.getCause ex)
282+
code (grpc.status/get-code grpc-status)]
283+
(is (some? cause))
284+
(is (= code (:status (ex-data cause))))
285+
cause))
286+
265287
;;-----------------------------------------------------------------------------
266288
;; Scaletest Assemblies
267289
;;-----------------------------------------------------------------------------
@@ -680,18 +702,20 @@
680702
(deftest test-grpc-exception
681703
(let [client @(grpc.http2/connect {:uri (str "http://localhost:" (:port @test-env))})]
682704
(testing "Check that exceptions thrown on the server propagate back to the client"
683-
(is (thrown? java.util.concurrent.ExecutionException
684-
@(test.client/ShouldThrow client {})))
685-
(try
686-
@(test.client/ShouldThrow client {})
687-
(assert false) ;; we should never get here
688-
(catch java.util.concurrent.ExecutionException e
689-
(let [{:keys [status]} (ex-data (.getCause e))]
690-
(is (= status 13))))))
705+
(is-grpc-error? :internal (test.client/ShouldThrow client {})))
691706
(testing "Check that we can still connect even after exceptions have been received"
692707
(is (-> @(test.client/Async client {}) :msg (= "Hello, Async"))))
693708
(grpc/disconnect client)))
694709

710+
(deftest test-grpc-sequence-exception
711+
(let [client @(grpc.http2/connect {:uri (str "http://localhost:" (:port @test-env))})]
712+
(testing "Check that exceptions thrown in sequence mappers on the server propagate back to the client"
713+
(testing "Non-lazy: throws in mapv"
714+
(is-grpc-error? :internal (test.client/ShouldThrow client {:case 1})))
715+
(testing "Lazy: throws in map"
716+
(is-grpc-error? :internal (test.client/ShouldThrow client {:case 2}))))
717+
(grpc/disconnect client)))
718+
695719
(deftest test-grpc-async
696720
(testing "Check that async processing functions correctly"
697721
(let [client @(grpc.http2/connect {:uri (str "http://localhost:" (:port @test-env))})]

‎test/test/protojure/test/grpc.cljc‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@
3737
(declare cis->BigPayload)
3838
(declare ecis->BigPayload)
3939
(declare new-BigPayload)
40+
(declare cis->ShouldThrowRequest)
41+
(declare ecis->ShouldThrowRequest)
42+
(declare new-ShouldThrowRequest)
43+
(declare cis->ShouldThrowResponse)
44+
(declare ecis->ShouldThrowResponse)
45+
(declare new-ShouldThrowResponse)
4046

4147
;;----------------------------------------------------------------------------------
4248
;;----------------------------------------------------------------------------------
@@ -378,3 +384,99 @@
378384

379385
(def ^:protojure.protobuf.any/record BigPayload-meta {:type "protojure.test.grpc.BigPayload" :decoder pb->BigPayload})
380386

387+
;-----------------------------------------------------------------------------
388+
; ShouldThrowRequest
389+
;-----------------------------------------------------------------------------
390+
(defrecord ShouldThrowRequest-record [case]
391+
pb/Writer
392+
(serialize [this os]
393+
(serdes.core/write-Int32 1 {:optimize true} (:case this) os))
394+
pb/TypeReflection
395+
(gettype [this]
396+
"protojure.test.grpc.ShouldThrowRequest"))
397+
398+
(s/def :protojure.test.grpc.ShouldThrowRequest/case int?)
399+
(s/def ::ShouldThrowRequest-spec (s/keys :opt-un [:protojure.test.grpc.ShouldThrowRequest/case ]))
400+
(def ShouldThrowRequest-defaults {:case 0 })
401+
402+
(defn cis->ShouldThrowRequest
403+
"CodedInputStream to ShouldThrowRequest"
404+
[is]
405+
(->> (tag-map ShouldThrowRequest-defaults
406+
(fn [tag index]
407+
(case index
408+
1 [:case (serdes.core/cis->Int32 is)]
409+
410+
[index (serdes.core/cis->undefined tag is)]))
411+
is)
412+
(map->ShouldThrowRequest-record)))
413+
414+
(defn ecis->ShouldThrowRequest
415+
"Embedded CodedInputStream to ShouldThrowRequest"
416+
[is]
417+
(serdes.core/cis->embedded cis->ShouldThrowRequest is))
418+
419+
(defn new-ShouldThrowRequest
420+
"Creates a new instance from a map, similar to map->ShouldThrowRequest except that
421+
it properly accounts for nested messages, when applicable.
422+
"
423+
[init]
424+
{:pre [(if (s/valid? ::ShouldThrowRequest-spec init) true (throw (ex-info "Invalid input" (s/explain-data ::ShouldThrowRequest-spec init))))]}
425+
(-> (merge ShouldThrowRequest-defaults init)
426+
(map->ShouldThrowRequest-record)))
427+
428+
(defn pb->ShouldThrowRequest
429+
"Protobuf to ShouldThrowRequest"
430+
[input]
431+
(cis->ShouldThrowRequest (serdes.stream/new-cis input)))
432+
433+
(def ^:protojure.protobuf.any/record ShouldThrowRequest-meta {:type "protojure.test.grpc.ShouldThrowRequest" :decoder pb->ShouldThrowRequest})
434+
435+
;-----------------------------------------------------------------------------
436+
; ShouldThrowResponse
437+
;-----------------------------------------------------------------------------
438+
(defrecord ShouldThrowResponse-record [numbers]
439+
pb/Writer
440+
(serialize [this os]
441+
(serdes.complex/write-repeated serdes.core/write-Int32 1 (:numbers this) os))
442+
pb/TypeReflection
443+
(gettype [this]
444+
"protojure.test.grpc.ShouldThrowResponse"))
445+
446+
(s/def :protojure.test.grpc.ShouldThrowResponse/numbers (s/every int?))
447+
(s/def ::ShouldThrowResponse-spec (s/keys :opt-un [:protojure.test.grpc.ShouldThrowResponse/numbers ]))
448+
(def ShouldThrowResponse-defaults {:numbers [] })
449+
450+
(defn cis->ShouldThrowResponse
451+
"CodedInputStream to ShouldThrowResponse"
452+
[is]
453+
(->> (tag-map ShouldThrowResponse-defaults
454+
(fn [tag index]
455+
(case index
456+
1 [:numbers (serdes.complex/cis->packablerepeated tag serdes.core/cis->Int32 is)]
457+
458+
[index (serdes.core/cis->undefined tag is)]))
459+
is)
460+
(map->ShouldThrowResponse-record)))
461+
462+
(defn ecis->ShouldThrowResponse
463+
"Embedded CodedInputStream to ShouldThrowResponse"
464+
[is]
465+
(serdes.core/cis->embedded cis->ShouldThrowResponse is))
466+
467+
(defn new-ShouldThrowResponse
468+
"Creates a new instance from a map, similar to map->ShouldThrowResponse except that
469+
it properly accounts for nested messages, when applicable.
470+
"
471+
[init]
472+
{:pre [(if (s/valid? ::ShouldThrowResponse-spec init) true (throw (ex-info "Invalid input" (s/explain-data ::ShouldThrowResponse-spec init))))]}
473+
(-> (merge ShouldThrowResponse-defaults init)
474+
(map->ShouldThrowResponse-record)))
475+
476+
(defn pb->ShouldThrowResponse
477+
"Protobuf to ShouldThrowResponse"
478+
[input]
479+
(cis->ShouldThrowResponse (serdes.stream/new-cis input)))
480+
481+
(def ^:protojure.protobuf.any/record ShouldThrowResponse-meta {:type "protojure.test.grpc.ShouldThrowResponse" :decoder pb->ShouldThrowResponse})
482+

‎test/test/protojure/test/grpc/TestService/client.cljc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,8 +161,8 @@
161161
output (async/chan 1)
162162
desc {:service "protojure.test.grpc.TestService"
163163
:method "ShouldThrow"
164-
:input {:f com.google.protobuf/new-Empty :ch input}
165-
:output {:f com.google.protobuf/pb->Empty :ch output}
164+
:input {:f protojure.test.grpc/new-ShouldThrowRequest :ch input}
165+
:output {:f protojure.test.grpc/pb->ShouldThrowResponse :ch output}
166166
:metadata metadata}]
167167
(-> (send-unary-params input params)
168168
(p/then (fn [_] (invoke-unary client desc output)))))))

‎test/test/protojure/test/grpc/TestService/server.cljc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,4 +75,4 @@
7575
{:pkg "protojure.test.grpc" :service "TestService" :method "AsyncEmpty" :method-fn AsyncEmpty-dispatch :server-streaming true :client-streaming false :input com.google.protobuf/pb->Empty :output com.google.protobuf/new-Empty}
7676
{:pkg "protojure.test.grpc" :service "TestService" :method "Metadata" :method-fn Metadata-dispatch :server-streaming false :client-streaming false :input com.google.protobuf/pb->Empty :output new-SimpleResponse}
7777
{:pkg "protojure.test.grpc" :service "TestService" :method "ReturnErrorStreaming" :method-fn ReturnErrorStreaming-dispatch :server-streaming true :client-streaming false :input pb->ErrorRequest :output com.google.protobuf/new-Empty}
78-
{:pkg "protojure.test.grpc" :service "TestService" :method "ShouldThrow" :method-fn ShouldThrow-dispatch :server-streaming false :client-streaming false :input com.google.protobuf/pb->Empty :output com.google.protobuf/new-Empty}])
78+
{:pkg "protojure.test.grpc" :service "TestService" :method "ShouldThrow" :method-fn ShouldThrow-dispatch :server-streaming false :client-streaming false :input pb->ShouldThrowRequest :output new-ShouldThrowResponse}])

‎test/test/resources/grpctest.proto‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,25 @@ message BigPayload {
4040
bytes data = 2;
4141
}
4242

43+
message ShouldThrowRequest {
44+
int32 case = 1;
45+
}
46+
47+
message ShouldThrowResponse {
48+
repeated int32 numbers = 1;
49+
}
50+
4351
service TestService {
4452
rpc ClientCloseDetect (CloseDetectRequest) returns (stream google.protobuf.Any);
4553
rpc ServerCloseDetect (google.protobuf.Empty) returns (stream google.protobuf.Any);
4654
rpc FlowControl (FlowControlRequest) returns (stream FlowControlPayload);
4755
rpc Metadata (google.protobuf.Empty) returns (SimpleResponse);
48-
rpc ShouldThrow (google.protobuf.Empty) returns (google.protobuf.Empty);
56+
rpc ShouldThrow (ShouldThrowRequest) returns (ShouldThrowResponse);
4957
rpc Async (google.protobuf.Empty) returns (SimpleResponse);
5058
rpc AllEmpty(google.protobuf.Empty) returns (google.protobuf.Empty);
5159
rpc AsyncEmpty(google.protobuf.Empty) returns (stream google.protobuf.Empty);
5260
rpc DeniedStreamer(google.protobuf.Empty) returns (stream google.protobuf.Empty);
5361
rpc ReturnError(ErrorRequest) returns (google.protobuf.Empty);
5462
rpc ReturnErrorStreaming(ErrorRequest) returns (stream google.protobuf.Empty);
5563
rpc BandwidthTest(BigPayload) returns (BigPayload);
56-
}
64+
}

0 commit comments

Comments
 (0)