allow client/server to be initialised with a connected socket #1385
Conversation
|
|
|
re: #1353 (comment) I would just create a new test class, something like https://github.com/grpc/grpc-swift/blob/main/Tests/GRPCTests/GRPCKeepaliveTests.swift should be a useful starting point. |
|
@jvimal-eg you'll need to run Note that the formatter version was changed recently, so make sure you're branch is up-to-date with |
|
@glbrntt Sounds good. I am new to Swift / NIO, and I am stumped at how to create a |
|
It's a file descriptor on all currently supported platforms, so you can safely use the regular libc APIs to create the Unix domain socket. |
jvimal-eg
force-pushed
the
jv/uds-connected-socket
branch
from
a479e45 to
9adc755
Compare
|
@Lukasa thanks, done! |
| @@ -0,0 +1,72 @@ | |||
| /* | |||
| * Copyright 2020, gRPC Authors All rights reserved. | |||
There was a problem hiding this comment.
| * Copyright 2020, gRPC Authors All rights reserved. | |
| * Copyright 2022, gRPC Authors All rights reserved. |
| import EchoModel | ||
| @testable import GRPC | ||
| import NIOCore | ||
| @testable import NIOPosix |
There was a problem hiding this comment.
We can't do this I'm afraid: @testable allows us to use the internal API for NIOPosix which we shouldn't do as it could change at any time. Instead we should create the connected socket manually.
| XCTAssertThrowsError(try get.response.wait()) | ||
| XCTAssertEqual(try get.status.map { $0.code }.wait(), .unavailable) |
There was a problem hiding this comment.
Here we should check that the response is as we expect and that get.status.wait().isOk.
| class ReadDroppingHandler: ChannelDuplexHandler { | ||
| typealias InboundIn = Any | ||
| typealias OutboundIn = Any | ||
|
|
||
| func channelRead(context: ChannelHandlerContext, data: NIOAny) {} | ||
| } |
There was a problem hiding this comment.
We don't need this either.
| // See above comments for why we need this. | ||
| .withCallStartBehavior(.fastFailure) | ||
| .withKeepalive(.init(interval: .seconds(1), timeout: .milliseconds(100))) | ||
| .withDebugChannelInitializer { channel in | ||
| channel.pipeline.addHandler(ReadDroppingHandler(), position: .first) | ||
| } |
There was a problem hiding this comment.
We don't need this part (or the comment above), it's not relevant to this test. The test should just check we can do an RPC.
| // See above comments for why we need this. | |
| .withCallStartBehavior(.fastFailure) | |
| .withKeepalive(.init(interval: .seconds(1), timeout: .milliseconds(100))) | |
| .withDebugChannelInitializer { channel in | |
| channel.pipeline.addHandler(ReadDroppingHandler(), position: .first) | |
| } |
|
@glbrntt thanks for your review. Apologies -- I blindly copypasta the old test without really paying attention to what it was doing. I fixed the test now. |
No worries! You need to rebase your branch on main and reformat, see: #1385 (comment) |
jvimal-eg
force-pushed
the
jv/uds-connected-socket
branch
from
28ade48 to
df845b8
Compare
|
@glbrntt Thanks. Rebased, reformatted, and addressed your comments. |
) Motivation: grpc#1353 Modifications: - A new entry on ConnectionTarget - New API on the ClientConnection builder - Some validation that we're not trying to use this API with a NIOTSEventLoopGroup or NIOTSEventLoop (as far as I can tell, using a file descriptor directly is not possible with Network.framework) - Tests Result: This allows greater flexibility in spawning the client/server; in particular, it allows unix domain sockets for sandboxed apps on macOS.
Motivation: #1353
Modifications: As outlined by @glbrntt:
here (the channel pool uses ConnectionTarget directly so does not need new API)
NIOTSEventLoopGroup or NIOTSEventLoop (as far as I can tell, using a file
descriptor directly is not possible with Network.framework)
Result:
This allows greater flexibility in spawning the client/server; in particular, it allows unix domain sockets for sandboxed apps on macOS.