utrack / clay

Proto-first minimal server platform for gRPС+REST+Swagger APIs
MIT License
289 stars 39 forks source link

Add test reproducing panic during marshaling response #49

Closed doroginin closed 5 years ago

utrack commented 5 years ago

This way, we're buffering the whole response in memory instead of dumping it right away - that can eat quite a lot of memory and CPU for byte allocations. I'm not sure which panic that'll fix except if a writer is nil; can you show me the stacktrace, please?

doroginin commented 5 years ago

for example this:

2018/10/05 14:44:14 http: panic serving 127.0.0.1:34892: interface conversion: *time.Time is not proto.Message: missing method ProtoMessage
goroutine 34 [running]:
net/http.(*conn).serve.func1(0xc0004fc000)
        /usr/local/go/src/net/http/server.go:1746 +0xd0
panic(0x960f20, 0xc0004b2450)
        /usr/local/go/src/runtime/panic.go:513 +0x1b9
github.com/golang/protobuf/jsonpb.(*Marshaler).marshalValue(0xe8c440, 0xc0004e7890, 0xc0005000c0, 0x9fb140, 0xc00050a0c0, 0x199, 0x0, 0x0, 0xc0004e72d8, 0xc0005000c0)
        /home/dd/go/pkg/mod/github.com/golang/protobuf@v1.2.0/jsonpb/jsonpb.go:554 +0xbbd
github.com/golang/protobuf/jsonpb.(*Marshaler).marshalField(0xe8c440, 0xc0004e7890, 0xc0005000c0, 0x9fb140, 0xc00050a0c0, 0x199, 0x0, 0x0, 0x0, 0xc00043e0c0)
        /home/dd/go/pkg/mod/github.com/golang/protobuf@v1.2.0/jsonpb/jsonpb.go:475 +0x107
github.com/golang/protobuf/jsonpb.(*Marshaler).marshalObject(0xe8c440, 0xc0004e7890, 0xaa1ac0, 0xc00050a0c0, 0x0, 0x0, 0x0, 0x0, 0x7feb75466d48, 0x4564c0)
        /home/dd/go/pkg/mod/github.com/golang/protobuf@v1.2.0/jsonpb/jsonpb.go:333 +0x4fe
github.com/golang/protobuf/jsonpb.(*Marshaler).Marshal(0xe8c440, 0x7feb75404880, 0xc0004b2270, 0xaa1ac0, 0xc00050a0c0, 0xc00044c001, 0x3a85e980a0a3c83f)
        /home/dd/go/pkg/mod/github.com/golang/protobuf@v1.2.0/jsonpb/jsonpb.go:138 +0x1ad
github.com/grpc-ecosystem/grpc-gateway/runtime.(*JSONPb).marshalTo(0xe8c440, 0x7feb75404880, 0xc0004b2270, 0x9b7060, 0xc00050a0c0, 0x20, 0x991000)
        /home/dd/go/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.5.0/runtime/marshal_jsonpb.go:50 +0x110
github.com/grpc-ecosystem/grpc-gateway/runtime.(*JSONPb).NewEncoder.func1(0x9b7060, 0xc00050a0c0, 0x991000, 0xc00050a120)
        /home/dd/go/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.5.0/runtime/marshal_jsonpb.go:115 +0x50
github.com/grpc-ecosystem/grpc-gateway/runtime.EncoderFunc.Encode(0xc00050a120, 0x9b7060, 0xc00050a0c0, 0xa9c620, 0xc00050a120)
        /home/dd/go/pkg/mod/github.com/grpc-ecosystem/grpc-gateway@v1.5.0/runtime/marshaler.go:42 +0x3a
github.com/utrack/clay/v2/transport/httpruntime.MarshalerPbJSON.Marshal(0xe8c440, 0xe8c480, 0xe8c4c0, 0xe8b640, 0x7feb75404880, 0xc0004b2270, 0x9b7060, 0xc00050a0c0, 0xa0c300, 0x7feb75404880)
        /home/dd/data/Documents/dev/src/github.com/utrack/clay/transport/httpruntime/mjson.go:68 +0xb2
github.com/utrack/clay/integration/panic_in_response_marshaler/pb.(*StringsDesc).RegisterHTTP.func1(0xaa1b40, 0xc0004b2270, 0xc00050c200)
        /home/dd/data/Documents/dev/src/github.com/utrack/clay/integration/panic_in_response_marshaler/pb/strings.pb.goclay.go:128 +0x3ff
net/http.HandlerFunc.ServeHTTP(0xc000427c00, 0xaa1b40, 0xc0004b2270, 0xc00050c200)
        /usr/local/go/src/net/http/server.go:1964 +0x44
github.com/utrack/clay/v2/transport/httpruntime/httpmw.HeadersToGRPCMD.func1(0xaa1b40, 0xc0004b2270, 0xc00050c100)
        /home/dd/data/Documents/dev/src/github.com/utrack/clay/transport/httpruntime/httpmw/headers.go:45 +0x211
net/http.HandlerFunc.ServeHTTP(0xc000427c10, 0xaa1b40, 0xc0004b2270, 0xc00050c100)
        /usr/local/go/src/net/http/server.go:1964 +0x44
github.com/utrack/clay/v2/transport/httpruntime/httpmw.InjectTransportStream.func1(0xaa2080, 0xc0000f0000, 0xc00050c000)
        /home/dd/data/Documents/dev/src/github.com/utrack/clay/transport/httpruntime/httpmw/headers.go:57 +0x1b2
github.com/utrack/clay/integration/panic_in_response_marshaler/pb.(*StringsDesc).RegisterHTTP.func2(0xaa2080, 0xc0000f0000, 0xc00050c000)
        /home/dd/data/Documents/dev/src/github.com/utrack/clay/integration/panic_in_response_marshaler/pb/strings.pb.goclay.go:147 +0x7c
net/http.HandlerFunc.ServeHTTP(0xc000427c30, 0xaa2080, 0xc0000f0000, 0xc00050c000)
        /usr/local/go/src/net/http/server.go:1964 +0x44
net/http.(*ServeMux).ServeHTTP(0xc000495650, 0xaa2080, 0xc0000f0000, 0xc00050c000)
        /usr/local/go/src/net/http/server.go:2361 +0x127
net/http.serverHandler.ServeHTTP(0xc0003b8ea0, 0xaa2080, 0xc0000f0000, 0xc00050c000)
        /usr/local/go/src/net/http/server.go:2741 +0xab
net/http.(*conn).serve(0xc0004fc000, 0xaa2980, 0xc0004ae140)
        /usr/local/go/src/net/http/server.go:1847 +0x646
created by net/http.(*Server).Serve
        /usr/local/go/src/net/http/server.go:2851 +0x2f5
utrack commented 5 years ago

I'm not sure how adding an intermediate buffer would fix that. This is a problem b/c of golang/jsonpb that is used for gogo/jsonpb generated code, changing write destination won't fix anything.

doroginin commented 5 years ago

Changing write destination fixes ResponseWriter buffer, because now it can have broken json:

{
  "time_field": {
    "error":"recovered from panic: interface conversion: *time.Time is not proto.Message: missing method ProtoMessage"
  }

This json does'n have closing }

And even we will recover this panic we can't modify already written broken json.

utrack commented 5 years ago

Yeah, but it's a hack, not a fix.

doroginin commented 5 years ago

Ok, i reverted this changes. Instead of this I added test reproducing panic problem, also binding_with_different_types_post test was added (the same as binding_with_different_types but for http POST and with WKT types)

doroginin commented 5 years ago

To prevent panic in case of using: google.protobuf.Timestamp stdtime = 19 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false]; 1) You should use gogofast protoc plugin or use option (gogoproto.gogoproto_import) = true; option in your proto. 2) If you would like to use reflection.Register for your service, you should also use this option in your proto: option (gogoproto.goproto_registration) = true;