Send a status with initial metadata

[KyoheiG3]

I think it would be better to implement the following methods.
https://github.com/grpc/grpc-swift/blob/0.4.3/Sources/SwiftGRPC/Runtime/ServiceServer.swift#L88-L97

For example, extend sendStatus of Handler method so that initialMedadata can be sent?

public func sendStatus(_ status: ServerStatus, initialMetadata: Metadata? = nil, completion: (() -> Void)? = nil) throws

If you want to need proposal, I will create PR.

[MrMage]

Thank you for the suggestion! My I ask what your goals are with such a change? I.e. when do you need to send custom initialMetadata?

In general, the request flow in Swift GRPC is this:

1. The server receives the request and checks if it finds a handler matching the given method.
2. If a handler is found, the server sends initial metadata (normally equivalent to "OK, I received your request").
3. The handler is called.
4. If the handler errors out, an error status is sent back to the client. If the error is of type ServerStatus, its trailingMetadata is sent as well.
5. If the handler is successful, it just sends ServerStatus.ok.

If we were to extend this, I think it would make the most sense to improve support for sending custom trailing metadata, as the initial metadata is sent very early, before the actual handler implementation is even called.

[KyoheiG3]

I understand. It is not good to extend sendStatus. My goal is more simply.

In func start() that ServiceServer has, it send a status using func sendStatus(_ status: ServerStatus, ...) normally. But in catch as HandleMethodError, it send a status using perform method that Call has. I think that it is not good to use perform method directly and it is better to implement method that send sendInitialMetadata, receiveCloseOnServer and sendStatusFromServer together.

Please reference ServiceServer.swift#L88-L97.

[MrMage]

But that branch is only called when there is no handler to handle a method with the given name. Why do you need to send metadata in that case??

[KyoheiG3]

I just want to change following:

try handler.call.perform(OperationGroup(
    call: handler.call,
    operations: [
    .sendInitialMetadata(Metadata()),
    .receiveCloseOnServer,
    .sendStatusFromServer(.unimplemented, "unknown method " + unwrappedMethod, Metadata())
]) { _ in
    handler.shutdown()
})

↓

let status = ServerStatus(code: .unimplemented, message: "unknown method " + unwrappedMethod)
try handler.sendStatus(status)

Thanks.

[MrMage]

Thanks! Now I understand what you mean.

While https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses makes it sound like sending no header should be okay, https://github.com/grpc/grpc/blob/f0375f86e94f9970283e80996e1326b4416ed4da/include/grpc/impl/codegen/grpc_types.h#L487 makes it sound like .sendInitialMetadata must be called. So in this case, I would rather err on the side of sending the initial metadata to avoid confusing errors down the road; especially given that the only benefit would be slightly cleaner code in an area that is rarely touched anyway.

[KyoheiG3]

Thank you for understanding.

So, are not you planning to implement a dedicated method to send .unimplemented status on Handler?

[MrMage]

What would be the benefit of that?

…

On 7. Jul 2018, at 18:18, Kyohei Ito ***@***.***> wrote: Thank you for understanding. So, are not you planning to implement a dedicated method to send .unimplemented status on Handler? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[KyoheiG3]

Core components can be hidden more. However, I think this is not a big benefit.

I think Core should be more privately and Runtime should be more publicly.
By creating that method, OperationGroup and Operation in Core are not referenced from Runtime.

[MrMage]

Thanks for the explanation! I would still keep the option open for users of SwiftGRPC to build their own, lower-level SwiftGRPC server, so I would like to not reduce the visibility of our methods.

However, I would not be opposed to adding one method to Core to do this, but we should be careful not to get too deep into bikeshedding. This is especially true given that Core and Runtime are part of the same Swift target, anyway.

If you are interested in further improving the quality of SwiftGRPC, I'd encourage you to look into the ideas listed in #189.

[KyoheiG3]

Thank you for the discussion. I'll check the issue.

[MrMage]

Closing this now; please open a new issue if this is still relevant for the nio branch.