server/rpc_codec: if c.codec.Write fails, reset write buffer and encode an error message about the encoding failure
When developing go-micro services, it is frequently possible to set invalid results in the response pointer. When this happens (as I and @trushton personally experienced), `sendResponse()` returns an error correctly explaining what happened (e.g. protobuf refused to encode a bad struct) but the `call()` function one above it in the stack ignores the returned error object.
Thus, invalid structs go un-encoded and the _client side times out_. @trushton and I first caught this in our CI builds when we left a protobuf.Empty field uninitialized (nil) instead of setting it to `&ptypes.Empty{}`. This resulted in an `proto: oneof field has nil value` error, but it was dropped and became a terribly confusing client timeout instead.
This patch is two independent changes:
* In rpc_codec, when a serialization failure occurs serialize an error message, which will correctly become a 500 for HTTP services, about the encoding failure. This means rpc_codec only returns an `error` when a socket failure occurs, which I believe is the behavior that rpc_service is expecting anyway.
* In rpc_service, log any errors returned by sendResponse instead of dropping the error object. This will make debugging client timeouts less of a hassle.
2017-07-11 17:51:36 -04:00
|
|
|
package server
|
|
|
|
|
|
|
|
import (
|
|
|
|
"bytes"
|
|
|
|
"errors"
|
|
|
|
"testing"
|
|
|
|
|
2020-01-30 14:39:00 +03:00
|
|
|
"github.com/micro/go-micro/v2/codec"
|
|
|
|
"github.com/micro/go-micro/v2/transport"
|
server/rpc_codec: if c.codec.Write fails, reset write buffer and encode an error message about the encoding failure
When developing go-micro services, it is frequently possible to set invalid results in the response pointer. When this happens (as I and @trushton personally experienced), `sendResponse()` returns an error correctly explaining what happened (e.g. protobuf refused to encode a bad struct) but the `call()` function one above it in the stack ignores the returned error object.
Thus, invalid structs go un-encoded and the _client side times out_. @trushton and I first caught this in our CI builds when we left a protobuf.Empty field uninitialized (nil) instead of setting it to `&ptypes.Empty{}`. This resulted in an `proto: oneof field has nil value` error, but it was dropped and became a terribly confusing client timeout instead.
This patch is two independent changes:
* In rpc_codec, when a serialization failure occurs serialize an error message, which will correctly become a 500 for HTTP services, about the encoding failure. This means rpc_codec only returns an `error` when a socket failure occurs, which I believe is the behavior that rpc_service is expecting anyway.
* In rpc_service, log any errors returned by sendResponse instead of dropping the error object. This will make debugging client timeouts less of a hassle.
2017-07-11 17:51:36 -04:00
|
|
|
)
|
|
|
|
|
|
|
|
// testCodec is a dummy codec that only knows how to encode nil bodies
|
|
|
|
type testCodec struct {
|
|
|
|
buf *bytes.Buffer
|
|
|
|
}
|
|
|
|
|
|
|
|
type testSocket struct {
|
2018-11-14 19:45:46 +00:00
|
|
|
local string
|
|
|
|
remote string
|
server/rpc_codec: if c.codec.Write fails, reset write buffer and encode an error message about the encoding failure
When developing go-micro services, it is frequently possible to set invalid results in the response pointer. When this happens (as I and @trushton personally experienced), `sendResponse()` returns an error correctly explaining what happened (e.g. protobuf refused to encode a bad struct) but the `call()` function one above it in the stack ignores the returned error object.
Thus, invalid structs go un-encoded and the _client side times out_. @trushton and I first caught this in our CI builds when we left a protobuf.Empty field uninitialized (nil) instead of setting it to `&ptypes.Empty{}`. This resulted in an `proto: oneof field has nil value` error, but it was dropped and became a terribly confusing client timeout instead.
This patch is two independent changes:
* In rpc_codec, when a serialization failure occurs serialize an error message, which will correctly become a 500 for HTTP services, about the encoding failure. This means rpc_codec only returns an `error` when a socket failure occurs, which I believe is the behavior that rpc_service is expecting anyway.
* In rpc_service, log any errors returned by sendResponse instead of dropping the error object. This will make debugging client timeouts less of a hassle.
2017-07-11 17:51:36 -04:00
|
|
|
}
|
|
|
|
|
|
|
|
// TestCodecWriteError simulates what happens when a codec is unable
|
|
|
|
// to encode a message (e.g. a missing branch of an "oneof" message in
|
|
|
|
// protobufs)
|
|
|
|
//
|
|
|
|
// We expect an error to be sent to the socket. Previously the socket
|
|
|
|
// would remain open with no bytes sent, leading to client-side
|
|
|
|
// timeouts.
|
|
|
|
func TestCodecWriteError(t *testing.T) {
|
|
|
|
socket := testSocket{}
|
|
|
|
message := transport.Message{
|
|
|
|
Header: map[string]string{},
|
|
|
|
Body: []byte{},
|
|
|
|
}
|
|
|
|
|
|
|
|
rwc := readWriteCloser{
|
|
|
|
rbuf: new(bytes.Buffer),
|
|
|
|
wbuf: new(bytes.Buffer),
|
|
|
|
}
|
|
|
|
|
2018-11-23 20:05:31 +00:00
|
|
|
c := rpcCodec{
|
server/rpc_codec: if c.codec.Write fails, reset write buffer and encode an error message about the encoding failure
When developing go-micro services, it is frequently possible to set invalid results in the response pointer. When this happens (as I and @trushton personally experienced), `sendResponse()` returns an error correctly explaining what happened (e.g. protobuf refused to encode a bad struct) but the `call()` function one above it in the stack ignores the returned error object.
Thus, invalid structs go un-encoded and the _client side times out_. @trushton and I first caught this in our CI builds when we left a protobuf.Empty field uninitialized (nil) instead of setting it to `&ptypes.Empty{}`. This resulted in an `proto: oneof field has nil value` error, but it was dropped and became a terribly confusing client timeout instead.
This patch is two independent changes:
* In rpc_codec, when a serialization failure occurs serialize an error message, which will correctly become a 500 for HTTP services, about the encoding failure. This means rpc_codec only returns an `error` when a socket failure occurs, which I believe is the behavior that rpc_service is expecting anyway.
* In rpc_service, log any errors returned by sendResponse instead of dropping the error object. This will make debugging client timeouts less of a hassle.
2017-07-11 17:51:36 -04:00
|
|
|
buf: &rwc,
|
|
|
|
codec: &testCodec{
|
|
|
|
buf: rwc.wbuf,
|
|
|
|
},
|
|
|
|
req: &message,
|
|
|
|
socket: socket,
|
|
|
|
}
|
|
|
|
|
2019-01-08 15:38:25 +00:00
|
|
|
err := c.Write(&codec.Message{
|
2019-01-10 21:25:31 +00:00
|
|
|
Endpoint: "Service.Endpoint",
|
|
|
|
Id: "0",
|
|
|
|
Error: "",
|
2019-01-08 15:38:25 +00:00
|
|
|
}, "body")
|
server/rpc_codec: if c.codec.Write fails, reset write buffer and encode an error message about the encoding failure
When developing go-micro services, it is frequently possible to set invalid results in the response pointer. When this happens (as I and @trushton personally experienced), `sendResponse()` returns an error correctly explaining what happened (e.g. protobuf refused to encode a bad struct) but the `call()` function one above it in the stack ignores the returned error object.
Thus, invalid structs go un-encoded and the _client side times out_. @trushton and I first caught this in our CI builds when we left a protobuf.Empty field uninitialized (nil) instead of setting it to `&ptypes.Empty{}`. This resulted in an `proto: oneof field has nil value` error, but it was dropped and became a terribly confusing client timeout instead.
This patch is two independent changes:
* In rpc_codec, when a serialization failure occurs serialize an error message, which will correctly become a 500 for HTTP services, about the encoding failure. This means rpc_codec only returns an `error` when a socket failure occurs, which I believe is the behavior that rpc_service is expecting anyway.
* In rpc_service, log any errors returned by sendResponse instead of dropping the error object. This will make debugging client timeouts less of a hassle.
2017-07-11 17:51:36 -04:00
|
|
|
|
|
|
|
if err != nil {
|
2019-01-07 18:20:47 +00:00
|
|
|
t.Fatalf(`Expected Write to fail; got "%+v" instead`, err)
|
server/rpc_codec: if c.codec.Write fails, reset write buffer and encode an error message about the encoding failure
When developing go-micro services, it is frequently possible to set invalid results in the response pointer. When this happens (as I and @trushton personally experienced), `sendResponse()` returns an error correctly explaining what happened (e.g. protobuf refused to encode a bad struct) but the `call()` function one above it in the stack ignores the returned error object.
Thus, invalid structs go un-encoded and the _client side times out_. @trushton and I first caught this in our CI builds when we left a protobuf.Empty field uninitialized (nil) instead of setting it to `&ptypes.Empty{}`. This resulted in an `proto: oneof field has nil value` error, but it was dropped and became a terribly confusing client timeout instead.
This patch is two independent changes:
* In rpc_codec, when a serialization failure occurs serialize an error message, which will correctly become a 500 for HTTP services, about the encoding failure. This means rpc_codec only returns an `error` when a socket failure occurs, which I believe is the behavior that rpc_service is expecting anyway.
* In rpc_service, log any errors returned by sendResponse instead of dropping the error object. This will make debugging client timeouts less of a hassle.
2017-07-11 17:51:36 -04:00
|
|
|
}
|
|
|
|
|
|
|
|
const expectedError = "Unable to encode body: simulating a codec write failure"
|
|
|
|
actualError := rwc.wbuf.String()
|
|
|
|
if actualError != expectedError {
|
|
|
|
t.Fatalf(`Expected error "%+v" in the write buffer, got "%+v" instead`, expectedError, actualError)
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
func (c *testCodec) ReadHeader(message *codec.Message, typ codec.MessageType) error {
|
|
|
|
return nil
|
|
|
|
}
|
|
|
|
|
|
|
|
func (c *testCodec) ReadBody(dest interface{}) error {
|
|
|
|
return nil
|
|
|
|
}
|
|
|
|
|
|
|
|
func (c *testCodec) Write(message *codec.Message, dest interface{}) error {
|
|
|
|
if dest != nil {
|
|
|
|
return errors.New("simulating a codec write failure")
|
|
|
|
}
|
|
|
|
c.buf.Write([]byte(message.Error))
|
|
|
|
return nil
|
|
|
|
}
|
|
|
|
|
|
|
|
func (c *testCodec) Close() error {
|
|
|
|
return nil
|
|
|
|
}
|
|
|
|
|
|
|
|
func (c *testCodec) String() string {
|
|
|
|
return "string"
|
|
|
|
}
|
|
|
|
|
2018-11-14 19:45:46 +00:00
|
|
|
func (s testSocket) Local() string {
|
|
|
|
return s.local
|
|
|
|
}
|
|
|
|
|
|
|
|
func (s testSocket) Remote() string {
|
|
|
|
return s.remote
|
|
|
|
}
|
|
|
|
|
server/rpc_codec: if c.codec.Write fails, reset write buffer and encode an error message about the encoding failure
When developing go-micro services, it is frequently possible to set invalid results in the response pointer. When this happens (as I and @trushton personally experienced), `sendResponse()` returns an error correctly explaining what happened (e.g. protobuf refused to encode a bad struct) but the `call()` function one above it in the stack ignores the returned error object.
Thus, invalid structs go un-encoded and the _client side times out_. @trushton and I first caught this in our CI builds when we left a protobuf.Empty field uninitialized (nil) instead of setting it to `&ptypes.Empty{}`. This resulted in an `proto: oneof field has nil value` error, but it was dropped and became a terribly confusing client timeout instead.
This patch is two independent changes:
* In rpc_codec, when a serialization failure occurs serialize an error message, which will correctly become a 500 for HTTP services, about the encoding failure. This means rpc_codec only returns an `error` when a socket failure occurs, which I believe is the behavior that rpc_service is expecting anyway.
* In rpc_service, log any errors returned by sendResponse instead of dropping the error object. This will make debugging client timeouts less of a hassle.
2017-07-11 17:51:36 -04:00
|
|
|
func (s testSocket) Recv(message *transport.Message) error {
|
|
|
|
return nil
|
|
|
|
}
|
|
|
|
|
|
|
|
func (s testSocket) Send(message *transport.Message) error {
|
|
|
|
return nil
|
|
|
|
}
|
|
|
|
|
|
|
|
func (s testSocket) Close() error {
|
|
|
|
return nil
|
|
|
|
}
|