[feature] Client waiting for end-of-rpc is clunky

[glbrntt]

In the NIO-based client, waiting for the RPC to terminate via the status future is a little clumsy:

• The documentation states that it should never be failed, so if we wait() it should not throw, but we still have to handle that case. try! can be used but is a code-smell.
• When the status is returned the user still has to check whether the status code represents an error state (i.e. status.code != .ok).

In a fully blown case of waiting to check whether an RPC terminates successfully we have something like:

let call = // ... make RPC call

do {
  let status = try call.status.wait()
  if status.code == .ok {
    // Great, RPC was successful!
  } else {
    // Uh-oh: RPC failed and gRPC Swift handled it (i.e. failed promises, closed channel)
  }
} catch {
  // Really bad: RPC failed and gRPC Swift didn't handle it, despite documenting that it would.
}

[mattpaletta]

Hi, I've used this project a while ago and I'd like to get involved. From what I can discern, adding this feature would mean wrapping this line in Swift-NIO:

[EventLoopFuture.swift, Line 885] extension EventLoopFuture { public func wait(file: StaticString = #file, line: UInt = #line) throws -> Value; }

Which is called from this line in swift-grpc:

[ServerStreamingCalls.swift, Line 72] public var status: EventLoopFuture<GRPCStatus>

Which returns a new EventLoopFuture. As a proposal, if swift-grpc had a 'wrapper' class that extended EventLoopFuture<GRPCStatus>, but overloaded the 'wait' class, then swift-grpc could wrap the error, meaning that status.wait wouldn't throw.

However, I recognize that this would break existing code, is it still worth fixing?

Would love your feedback, thanks.

[glbrntt]

hey @mattpaletta -- glad you'd like to get involved!

I think the solution to this is not changing existing behaviour, but adding convenience methods to the call objects. I think something like the following could be useful:

/// Registers a callback which is fired when `status` is resolved and has status code '.ok'
func whenSuccess(_ callback: () -> ())

/// Registers a callback which is fired when `status` is resolved and does not have status code '.ok'
func whenFailure(_ callback: (GRPCStatus) -> ())

/// Waits for 'status' to complete and throws the status if its code isn't '.ok'. This is essentially the blocking version of the two functions above.
func waitForCompletion() throws

[mattpaletta]

Hi @glbrntt,

Upon deeper inspection of the code, it seems that swift-nio already implements these methods and the EchoClient uses these methods (for checking response, but not Status).

 call.status.whenSuccess(callback: (GRPCStatus) -> Void)
 call.status.whenFailure(callback: (Error) -> Void)
 call.status.whenComplete(callback: (Result<GRPCStatus, Error>) -> Void)

I personally don't see the value of reimplementing this as part of the grpc-client directly, as that will lead to more duplicate code. You may feel differently.

I could instead change the echo-client to use the new callbacks when checking status, right now it uses:

let status = try collect.status.wait()

and then prints the status without checking it. Also happy to just close the issue.

[glbrntt]

The crux of the problem is not that the right behaviour can't be achieved, it's that it requires knowledge of the implementation.

This will never throw, because the implementation guarantees that we'll always succeed the underlying promise, however, users have to trust us our documented promise. We'll always get GRPCStatus and not an Error here:

let status = try call.status.wait()

However, the status does not always represent a successful outcome:

if status.isOk {
  // Great, we completed successfully!
} else {
  // Uh-oh, something wen't wrong.
}

This also relies on users knowing that the status future will be the last return value of the RPC. The proposed extensions above are just a way to provide a more obvious API which doesn't rely on documented guarantees.

I personally don't see the value of reimplementing this as part of the grpc-client directly, as that will lead to more duplicate code.

This wouldn't be reimplementing futures, it would just delegate to NIO's EventLoopFuture whenSuccess/whenFailure functions to register callbacks with a small amount of additional logic.

I don't think this is a big problem but it's not small enough to warrant closing the issue just yet.

[BrunoCerberus]

I've tried all of this, but i can't make a simple request, and worse, apparently call.status.wait() freezes the app

[Lukasa]

You must not call wait on your main thread, it will block the thread. Can you file a new issue to discuss what you're trying to do?