[ADDED] Report Encoding exceptions during unary calls#119
[ADDED] Report Encoding exceptions during unary calls#119Rkiouak wants to merge 1 commit intoprotojure:masterfrom
Conversation
bb1a3d2 to
8fd82c8
Compare
* 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 <mrkiouak@gmail.com>
8fd82c8 to
b4ff694
Compare
| " | ||
| [client params ch] | ||
| (-> (grpc/invoke client params) | ||
| (-> (grpc/invoke client (assoc params :unary? true)) |
There was a problem hiding this comment.
Thinking about this: I'm not sure that we should make this specific to unary. If something is malformed, its probably best to fail the call regardless of unary or streaming. Silently dropping is probably both wrong and confusing regardless of the type. Thoughts?
There was a problem hiding this comment.
I went back and forth on that… I think I did recall that if an exception is thrown in the streaming case, the pipeline breaks — but I can’t remember if we went back to make that more resilient. I’ll check and get back on this PR after I’m done traveling this evening.
My main concern was whether we’d want to fail a client call out entirely for a single bad message in the streaming case — I think we’d ideally try to gracefully continue in the streaming case, but I will check to see whether or not thats how we’ve implemented the client currently.
There was a problem hiding this comment.
In going back to test this without the :unary? check on the encode-promise, the test suite hangs on my local machine. I'd need to dig in further to understand why (I'm not certain if its a bug with the change to a p/all waiting on the pipeline to report, or a race condition.
When you get a moment, is there anything that jumps out to you about this change that would cause UTs to hang if the change is applied to streaming calls as well?
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 mrkiouak@gmail.com