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

Best practices for handling decode failures in a websocket connection #4795

Closed
ianbbqzy opened this issue Oct 4, 2024 · 4 comments · Fixed by #4819 or #4864
Closed

Best practices for handling decode failures in a websocket connection #4795

ianbbqzy opened this issue Oct 4, 2024 · 4 comments · Fixed by #4819 or #4864

Comments

@ianbbqzy
Copy link
Contributor

ianbbqzy commented Oct 4, 2024

📚 Documentation

(A clear and concise description of what the issue is.)

I wasn't able to find any documentation around handling decoding failures. I'm new to GRPC ecosystem and we have a GRPC server that needs to accept requests from clients that don't use GRPC. often times, our clients do not follow the best practices of GRPC and send invalid requests. such as

2024/10/02 11:02:05 ERROR: Failed to decode request: proto: (line 1:14): invalid google.protobuf.Timestamp value "2024-10-02T11.02.05.5880000Z"
2024/10/04 13:43:29 ERROR: Failed to decode request: proto: (line 1:190): invalid google.protobuf.Duration value "1.541S"

This a developer error but the problem I'm running into is that there is no easy way for us to communicate with our clients of these issues. As a results, these requests just get dropped in our websocket streaming connection and never reach our GRPC servers. So everytime, this happens, we can only retroactively inform our users of these issues.

Any advice on this issue would be greatly appreciated!

@ianbbqzy ianbbqzy changed the title Best practices for handling decode failures Best practices for handling decode failures in a websocket connection Oct 4, 2024
@johanbrandhorst
Copy link
Collaborator

Hi, this isn't really the right forum for this, as we don't manage the decoder. You either want to ask in https://github.com/grpc/grpc-go or maybe https://github.com/protocolbuffers/protobuf-go. If you are using this software though, you might be interested in the custom error handler documentation: https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_your_gateway/#error-handler.

@ianbbqzy
Copy link
Contributor Author

ianbbqzy commented Oct 8, 2024

Hi Johan, thanks for the response. I dug a little deeper and I think the issue can be remedied in this repo at

if err := handleSend(); err != nil {

If I understand correctly, the decode error is logged but not exposed to the caller. What do you think? I can submit a pull request to do so if that makes sense to you. Also if you agree, I can also open a new feature request or bug report. Let me know if I'm missing something that prompted the original authors to handle the error the current way intentionally. Thanks!

@johanbrandhorst
Copy link
Collaborator

Sorry, I'm confused. I only just noticed that you were talking about websocket connections. Are you using the https://github.com/tmc/grpc-websocket-proxy to perform bidirectional streaming in the gateway? The gateway doesn't natively support websockets, and as you noticed bidirectional streaming is pretty poorly supported. I'm happy to review a PR if you think you can make something work though.

@ianbbqzy
Copy link
Contributor Author

ianbbqzy commented Oct 9, 2024

correct that we are using https://github.com/tmc/grpc-websocket-proxy

ianbbqzy pushed a commit to ianbbqzy/grpc-gateway that referenced this issue Oct 9, 2024
ianbbqzy pushed a commit to ianbbqzy/grpc-gateway that referenced this issue Oct 10, 2024
ianbbqzy pushed a commit to ianbbqzy/grpc-gateway that referenced this issue Oct 10, 2024
johanbrandhorst pushed a commit that referenced this issue Oct 23, 2024
…ming (#4795) (#4819)

* feat: expose invalid argument error to clients in bidirectional streaming (#4795)

* fix for loop

* remove for loop

* revert to for loop

* move stream close to not block stream.header()

---------

Co-authored-by: Ian Lee <ianlee@Ians-MacBook-Pro-2.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants