Ensure reader is empty before calling Close #564
Conversation
This triggers an extra Read which will avoid #563 This is only an issue if the remote side is sending empty TChannel frames, which should only happen during streaming.
| "testing" | ||
|
|
||
| "io/ioutil" |
There was a problem hiding this comment.
should this be with the other "top-level" imports?
| @@ -72,6 +75,15 @@ func TestJSONInputOutput(t *testing.T) { | |||
| assert.Equal(t, 20756, outObj.Value) | |||
| } | |||
|
|
|||
| func TestReadNotEmpty(t *testing.T) { | |||
| // Note: The contents need to be larger than the default buffer size of bufio.NewReader. | |||
There was a problem hiding this comment.
Is there a constant we can pull for the default buffer size (as opposed to 10000)?
There was a problem hiding this comment.
Nope, no constant we can use here.
There was a problem hiding this comment.
I don't really think it adds much value to have a constant that's only used in a single test, and doesn't represent anything (e.g., it's counting the number of repeats for the string, not a number of bytes). It's pretty arbitrary.
| @@ -72,6 +75,15 @@ func TestJSONInputOutput(t *testing.T) { | |||
| assert.Equal(t, 20756, outObj.Value) | |||
| } | |||
|
|
|||
| func TestReadNotEmpty(t *testing.T) { | |||
| // Note: The contents need to be larger than the default buffer size of bufio.NewReader. | |||
| r := bytes.NewReader([]byte("{}" + strings.Repeat("{}\n", 10000))) | |||
There was a problem hiding this comment.
so the "{}" is the 'end' of the thrift request right? Which is why anything after the first '{}' causes an error?
There was a problem hiding this comment.
in this case it's using JSON, but yep.
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "github.com/uber/tchannel-go/testutils/testreader" |
| // EnsureEmpty ensures that the specified reader is empty. If the reader is | ||
| // not empty, it returns an error with the specified stage in the message. | ||
| func EnsureEmpty(r io.Reader, stage string) error { | ||
| buf := make([]byte, 128) |
There was a problem hiding this comment.
should this just be,
var buff [128]byte
n, err := r.Read(buff[:])
There was a problem hiding this comment.
It doesn't really matter, buf escapes either way.
31 var buf [128]byte
32 n, err := r.Read(buf[:])> go build -gcflags=-m
# github.com/uber/tchannel-go/internal/argreader
./empty.go:32: buf escapes to heap
./empty.go:31: moved to heap: buf
Decided to instead pool the byte slices instead.
This triggers an extra Read which will avoid #563 This is only an issue if the remote side is sending empty TChannel frames, which should only happen during streaming.
This triggers an extra Read which will avoid
#563
This is only an issue if the remote side is sending empty TChannel
frames, which should only happen during streaming.
Fixes #563
cc @willhug @akshayjshah