Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADDED] Report Encoding exceptions during unary calls #119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Rkiouak
Copy link
Member

@Rkiouak Rkiouak commented Feb 15, 2022

  • 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 [email protected]

@Rkiouak Rkiouak requested a review from ghaskins February 15, 2022 20:37
@Rkiouak Rkiouak force-pushed the report-unary-encoding-ex branch 2 times, most recently from bb1a3d2 to 8fd82c8 Compare February 15, 2022 20:50
  * 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 <[email protected]>
@Rkiouak Rkiouak force-pushed the report-unary-encoding-ex branch from 8fd82c8 to b4ff694 Compare February 15, 2022 20:51
@@ -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))
Copy link
Member

@ghaskins ghaskins Feb 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants