-
Notifications
You must be signed in to change notification settings - Fork 420
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
Improve error handling in NIO server. #364
Conversation
- Adds a user-configurable error handler to the server - Updates NIO server codegen to provide an optional error handler - Errors are handled by GRPCChannelHandler or BaseCallHandler, depending on the pipeline state - Adds some error handling tests - Tidies some logic in HTTP1ToRawGRPCServerCodec - Extends message handling logic in HTTP1ToRawGRPCServerCodec to handle messages split across multiple ByteBuffers (i.e. when a message exceeds a the size of a frame)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! This is just great, and brings the server part quite a bit closer to production-readiness. Thank you so much!
I mostly have nits and clarification questions. Feel free to dispute any, especially those that end with a question mark ;-)
Sources/SwiftGRPCNIO/CallHandlers/ServerStreamingCallHandler.swift
Outdated
Show resolved
Hide resolved
Two more thoughts:
|
This should be pretty straightforward to do. I'll include this in the next revision.
This is potentially useful for us. However, with the new |
Tests/SwiftGRPCNIOTests/GRPCChannelHandlerResponseCapturingTestCase.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 work overall! Just a few minor changes left.
This is potentially useful for us. However, with the new
enum
of error cases, each one is only thrown in one or two places so I think we can leave this for another day.
The advantage of this would be that we would know the name of the gRPC method that caused the failure (e.g. "Get/Collect/Expand/Update"). Let's leave this for the future, but please add a TODO asking for this to ServerErrorDelegate
.
Looking good! @glbrntt, I think all that's left would be comments from you on the two remaining unresolved discussions and fixing the merge conflict. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, save for the missing commas.
Again, terrific work!
Looks like we have a fatal error in the NIO Web tests:
@MrMage given these tests weren't included in LinuxMain before do you have any objections to just disabling the failing test (and opening an issue) for now? |
Thanks for spotting that! I agree with disabling them for now and opening an issue. This is probably not even related to our code directly, but rather due to a flaw in the Linux implementation of |
depending on the pipeline state
to handle messages split across multiple ByteBuffers (i.e. when a
message exceeds a the size of a frame)