First pass implementation of NIO client #357
Conversation
|
|
||
| let requestHead = makeRequestHead(path: path, host: client.host) | ||
| subchannel.whenSuccess { channel in | ||
| channel.write(GRPCClientRequestPart<Request>.head(requestHead), promise: nil) |
There was a problem hiding this comment.
I don't think it makes sense to do that here; you'd have to flush and wait for each part to be sent to the network before sending the next.
There was a problem hiding this comment.
Unresolving this until we have fully discussed this on the other comment.
| import NIO | ||
| import NIOHTTP2 | ||
|
|
||
| public final class GRPCClient { |
There was a problem hiding this comment.
Make this (and others?) open, so that library users can modify behaviors as needed on a low level?
|
Thank you for this! As discussed, I don’t have time in the next few weeks to review this in depth, sorry :-/ I did leave a few drive-by comments, though; feel free to disregard them if they don’t make sense. The general approach looks sound to me and some parts are already better-engineered than my server implementation; hopefully we can improve the server based on this later on. Also, we would need tests, of course, but I could imagine you’d want the API to be fleshed our first. @rebello95, what do you think? |
|
Sorry, just saw the todo-list entwining Tests. Extra client arguments could come in a later PR; currently I’m favoring an array of parameterized enemy with all the possible call options. |
|
Thanks for the drive-by comments; no problem with not having the time to go into depth. |
|
Great idea! 😊
… On 23. Jan 2019, at 02:02, George Barnett ***@***.***> wrote:
@glbrntt commented on this pull request.
In Sources/SwiftGRPCNIO/ClientCalls/ServerStreamingClientCall.swift:
> + * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import Foundation
+import SwiftProtobuf
+import NIO
+
+public class ServerStreamingClientCall<RequestMessage: Message, ResponseMessage: Message>: BaseClientCall<RequestMessage, ResponseMessage> {
+
Agh, thanks, I'll hunt these down. There might be some value in having a style guide (contributing guidelines could mention it); might save on nit-picks 😉.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Sorry for the late reply - let's chat more in the phone call tomorrow. Happy to take a closer look after! |
This is meant to be on the TODO list. Adding it to the client should be relatively straightforward with SwiftNIO SSL. "Relatively" because there's also NIO Transport Services for platforms with |
| public class ServerStreamingClientCall<RequestMessage: Message, ResponseMessage: Message>: BaseClientCall<RequestMessage, ResponseMessage> { | ||
|
|
||
| public init(client: GRPCClient, path: String, request: RequestMessage, handler: @escaping (ResponseMessage) -> Void) { | ||
| super.init(channel: client.channel, multiplexer: client.multiplexer, responseHandler: .callback(handler: handler)) |
There was a problem hiding this comment.
Should the responseHandler be held weak?
There was a problem hiding this comment.
Probably up to the caller to use [weak self] when specifying the contents of the handler since we need to keep the closure around, no?
- 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)
glbrntt
changed the title
|
Thanks for your patience, George! I hope to finally get this and #364 reviewed next week. |
| /// with an `.end` event. | ||
| /// | ||
| /// - Parameter event: event to send. | ||
| func send(_ event: StreamEvent<RequestMessage>) |
There was a problem hiding this comment.
Hmm, good point. Fulfilling the promise only when we send the bytes to the network helps with regards to backpressure, though. If we e.g. want to stream 10k messages of 10kB each, we don't want to dump them all at once onto the NIO layer (and lower). Not sure what to do here; leaving this open. Maybe @Lukasa or @weissi could comment on this.
Also, could you elaborate the "(It would also require a flush after writing each message if the end was added to the same queue.)" part?
|
Sorry, I had just forgotten the correct name. Either is fine for me.
… On 28. Feb 2019, at 15:42, George Barnett ***@***.*** ***@***.***>> wrote:
@glbrntt commented on this pull request.
In Sources/SwiftGRPCNIO/ClientCalls/ClientCall.swift:
> + case .message(let message):
+ subchannel.whenSuccess { channel in
+ channel.write(NIOAny(GRPCClientRequestPart<RequestMessage>.message(message)), promise: nil)
+ }
+
+ case .end:
+ subchannel.whenSuccess { channel in
+ channel.writeAndFlush(NIOAny(GRPCClientRequestPart<RequestMessage>.end), promise: nil)
+ }
+ }
+ }
+}
+
+/// A `ClientCall` with a unary response; i.e. unary and client-streaming.
+public protocol UnaryResponseClientCall: ClientCall {
+ var response: EventLoopFuture<ResponseMessage> { get }
I'll leave it as is for now.
Is GRPCStatusRepresentable a renaming suggestion (Currently it's GRPCStatusTransformable)? I think I prefer representable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
In the PR description you mean, "Message encoding" is left. Does that refer to compression? |
Sources/SwiftGRPCNIO/ClientCalls/ServerStreamingClientCall.swift
Outdated
Show resolved
Hide resolved
|
Sorry, I just realised you were suggesting two changes:
... a "feed" method that "throws as many bytes as possible into the funnel"
I understood this to be "consume all of the bytes we can, even if we can't produce a message", which we would have to write into a buffer. As opposed to "read as many usable bytes as we can" which we're doing now (i.e we can still have bytes leftover if the message length is split across buffers).
You were reading me correctly there. We *would* need to copy all the leftover bytes into an internal buffer. However, let’s just end this discussion here; the current implementation is good.
|
Yes, I'll clarify. |
Sources/SwiftGRPCNIO/ClientCalls/ClientStreamingClientCall.swift
Outdated
Show resolved
Hide resolved
|
🎉 I will merge this once CI has finished. |
|
🚀 |
@MrMage @timburks
Notes:
GRPCClientand then pass it to a generated client for their service (i.e. `EchoClient){Unary,ClientStreaming,ServerStreaming,BidirectionalStreaming}ClientCall{Unary,ClientStreaming}ClientCallhaveresponsefutures{ServerStreaming,BidirectionalStreaming}ClientCallhave response callbacks{ClientStreaming,BidirectionalStreaming}ClientCallhavesend()methodsEchoProviderNIO.swifttaken from Add gRPC Web support. #350)Todo:
cancel()calls