ikhoon
commented
1 year ago This behavior makes adding both on-responseHeaders and on-responseTrailers callbacks not an option for me because the on-responseHeaders will be invoked too late so that the circuit breaker fails to short-circuit the next client stub invocation
If trailers-only is sent due to an error on the server side, the response stream is immediately closed. Hence, onResponseHeaders() will be called consecutively and the decision of the rule would be reported to the circuit breaker. Although we need to use two callbacks to properly extract gRPC status from trailers, I don't see the failure report using trailers-only is delayed.
Make Armeria be aware of such "trailers-only" response.
It is a nice suggestion. It is tricky for users to extract grpc-status from trailers. They have to know how gRPC wire format. Like onStatus(...) callback, we can create GrpcCircuitBreakerRule and add a onGrpcStatus() to it.
Lincong
minwoox
rhodo
Context
In my setup, Armeria + unary gRPC are used. On the client side, I decorate the
ClientBuilderwithCircuitBreakerandCircuitBreakerRulein order to set up circuit breaking based on gRPC status code (e.g. when gRPC status code != OK, count the request as a failure).Initially I passed a callback to
CircuitBreakerRule.builder().onResponseTrailers(...)and expect the callback to be invoked when HTTP-trailers are received so that I can extract gRPC status code from the HTTP-trailers. However, I realized that when the server side throws an exception, server side does not send HTTP trailers (according to gRPC protocol specification). Instead, gRPC protocol specification names the response "trailers-only" which is physically represented as HTTP headers (and there is no body nor HTTP-trailers). Therefore in this case, the callback passed toCircuitBreakerRule.builder().onResponseTrailers(...)does not get invoked until after the exception is thrown to the client stub invocation level. When the callback is invoked, the trailers is empty (which is expected because gRPC's "trailers-only" response does not have HTTP-trailers).Temporary workaround
I changed
CircuitBreakerRule.builder().onResponseTrailers(...)toCircuitBreakerRule.builder().onResponseHeaders(...)and try to extract gRPC status from the response HTTP headers. It works for the case where the server throws an exception and the response contains only HTTP headers ("trailers-only" in gRPC's terminology) with gRPC status code.Issues
The temporary workaround only works for the circuit breaker to detect failures in trailers-only response. However, the circuit breaker is completely blind to detect any failure if the server side sends back a response with 3 parts, namely headers, request body, and trailers where trailers contain the gRPC status code (as documented in the gRPC protocol specification).
Correct me if I am wrong on this one: when a
CircuitBreakerRule.builder()has 2 callbacks registered, one fromonResponseTrailers(...)and the other fromonResponseHeaders(...), theonResponseHeaderscallback does not get invoked when HTTP headers are received. Instead, it is invoked when HTTP trailers are received or when Armeria claims that there will be no HTTP trailers. There is a flagrequiresResponseTrailerson eachCircuitBreakerRuleto decide which registered callbacks should be invoked. This behavior makes adding both on-responseHeaders and on-responseTrailers callbacks not an option for me because the on-responseHeaders will be invoked too late so that the circuit breaker fails to short-circuit the next client stub invocation.Proposed improvement
Issues # 2so that whenCircuitBreakerRule.builder()has both on-responseHeaders and on-responseTrailers callbacks registered, theon-responseHeader callbackis invoked immediately when HTTP headers are received. IMHO it does not make sense semantically to wait for trailers and then invoke theon-responseHeader callback.on-responseHeaders(with received HTTP headers) andon-responseTrailers(with nothing) immediately. Because Armeria supports gRPC as a first-class citizen, it is reasonable (to some extent at least) to make Armeria be able to interpret gRPC trailers-only.Let me know if I misunderstood anything and what you think. Thanks!