yarpc / yarpc-go

A message passing platform for Go
MIT License
403 stars 101 forks source link

`unexpected EOF` when gRPC inbound returns error with details #2106

Closed asido closed 2 years ago

asido commented 2 years ago

Describe the bug Caller receives error unexpected EOF when gRPC outbound returns yarpc error with details. Doesn't happen with HTTP transport.

To Reproduce Is the issue reproducible?

Steps to reproduce the behavior:

go.mod:

module yarpc-error

go 1.16

require go.uber.org/yarpc v1.58.0

main.go:

package main

import (
    "bytes"
    "context"
    "net"
    "time"

    "go.uber.org/yarpc"
    yarpctransport "go.uber.org/yarpc/api/transport"
    yarpcgrpc "go.uber.org/yarpc/transport/grpc"
    "go.uber.org/yarpc/yarpcerrors"
)

type UnaryHandlerFunc func(context.Context, *yarpctransport.Request, yarpctransport.ResponseWriter) error

func (f UnaryHandlerFunc) Handle(ctx context.Context, r *yarpctransport.Request, w yarpctransport.ResponseWriter) error {
    return f(ctx, r, w)
}

func main() {
    listener, err := net.Listen("tcp", "127.0.0.1:0")
    if err != nil {
        panic("unable to listen: " + err.Error())
    }

    ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Minute))
    defer cancel()

    transport := yarpcgrpc.NewTransport()
    if err := transport.Start(); err != nil {
        panic("unable to start transport: " + err.Error())
    }

    router := yarpc.NewMapRouter("yarpc-error")
    router.Register([]yarpctransport.Procedure{
        {
            Name:    "some-procedure",
            Service: "some-service",
            HandlerSpec: yarpctransport.NewUnaryHandlerSpec(UnaryHandlerFunc(func(context.Context, *yarpctransport.Request, yarpctransport.ResponseWriter) error {
                return yarpcerrors.
                    Newf(yarpcerrors.CodeInvalidArgument, "some error").
                    WithDetails([]byte("kaboom")) // <-- TRIGGERS BUG
            })),
            Encoding:  "raw",
            Signature: "signature",
        },
    })

    inbound := transport.NewInbound(listener)
    inbound.SetRouter(router)
    if err := inbound.Start(); err != nil {
        panic("unable to start inbound: " + err.Error())
    }

    outbound := transport.NewSingleOutbound(inbound.Addr().String())
    if err := outbound.Start(); err != nil {
        panic("unable to start outbound: " + err.Error())
    }

    _, err = outbound.Call(ctx, &yarpctransport.Request{
        Caller:    "some-caller",
        Service:   "some-service",
        Encoding:  "raw",
        Procedure: "some-procedure",
        Body:      bytes.NewReader([]byte("some-payload")),
    })
    if err == nil {
        panic("unexpected call success")
    }
    println(err.Error())
}
$ go run .
code:unknown message:unexpected EOF

Expected behavior

$ go run .
code:invalid-argument message:some error
gopalchouhan commented 2 years ago

yarpcerrors.WithDetails() expects the details as a byte array so depending on the encoding scheme, the correct marshled byte array has to be provided here. In this case, string bytes were directly provided which can not be unmarshled through proto, hence we see the error from proto during unmarshling. Try passing a valid proto byte array and it should work fine. For example, I tried the following and works fine: WithDetails(types.StringValue{Value: "abc"}.XXX_unrecognized)

However, for a better readability I strongly recommend to use protobuf.WithErrorDetails() for protobuf error. Example:

HandlerSpec: yarpctransport.NewUnaryHandlerSpec(
    UnaryHandlerFunc(
        func(context.Context, *yarpctransport.Request, yarpctransport.ResponseWriter) error {
            return protobuf.NewError(yarpcerrors.CodeInvalidArgument,
                "some error",
                protobuf.WithErrorDetails(&types.StringValue{Value: "abc"}),
        )
}))
gopalchouhan commented 2 years ago

Spoke to @asido offline and agreed to close this issue.