From 5c24ba91b55514fc40d374836c9395c74ba85ec6 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Tue, 2 Dec 2014 17:07:41 -0800 Subject: [PATCH 1/2] Support encoding.BinaryMarshaler types Sending time.Time values over libchan has resulted in panics. This is a core type and correct handling of the time type in libchan is important. By adding support for types that implement encoding.BinaryMarshaler, we get support for time.Time along with other types that implement this interface. At first, it was unclear why this correctly worked, but closer examination showed that this is supported within the codec package. The main bug was in the handling in the (*pipeReceiver).copyValue method and its spdy equivalent. The value copier really needs to be generalized and ported to work with any libchan implementation to avoid such bugs in the future. --- copy_test.go | 31 +++++++++++++++++++++++++++++++ inmem.go | 14 ++++++++++---- spdy/copy.go | 17 +++++++++++++---- spdy/copy_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 spdy/copy_test.go diff --git a/copy_test.go b/copy_test.go index 78d204e..17308a1 100644 --- a/copy_test.go +++ b/copy_test.go @@ -4,6 +4,7 @@ import ( "io" "net" "os" + "reflect" "runtime/pprof" "testing" "time" @@ -129,6 +130,36 @@ func TestByteStreamProxy(t *testing.T) { SpawnProxyTest(t, client, server, 1) } +func TestTypeTransmission(t *testing.T) { + // TODO(stevvooe): Ensure that libchan transports can all have this same + // test run against it. + + // Add problem types to this type definition. For now, we just care about + // time.Time. + type A struct { + T time.Time + } + + expected := A{T: time.Now()} + + receiver, sender := Pipe() + + go func() { + if err := sender.Send(expected); err != nil { + t.Fatalf("unexpected error sending: %v", err) + } + }() + + var received A + if err := receiver.Receive(&received); err != nil { + t.Fatalf("unexpected error receiving: %v", err) + } + + if !reflect.DeepEqual(received, expected) { + t.Fatalf("expected structs to be equal: %#v != %#v", received, expected) + } +} + func SpawnProxyTest(t *testing.T, client SendTestRoutine, server ReceiveTestRoutine, proxyCount int) { endClient := make(chan bool) endServer := make(chan bool) diff --git a/inmem.go b/inmem.go index 14c46ca..76cf7ef 100644 --- a/inmem.go +++ b/inmem.go @@ -1,6 +1,7 @@ package libchan import ( + "encoding" "encoding/binary" "errors" "io" @@ -312,6 +313,13 @@ func (w *pipeSender) copyValue(v interface{}) (interface{}, error) { return w.copyChannelMessage(val) case map[interface{}]interface{}: return w.copyChannelInterfaceMessage(val) + case encoding.BinaryMarshaler: + p, err := val.MarshalBinary() + if err != nil { + return nil, err + } + + return w.copyValue(p) default: if rv := reflect.ValueOf(v); rv.Kind() == reflect.Ptr { if rv.Elem().Kind() == reflect.Struct { @@ -419,10 +427,8 @@ func (w *pipeSender) copyChannelInterfaceMessage(m map[interface{}]interface{}) return mCopy, nil } func (w *pipeSender) copyStructure(m interface{}) (interface{}, error) { - v := reflect.ValueOf(m) - if v.Kind() == reflect.Ptr { - v = v.Elem() - } + v := reflect.Indirect(reflect.ValueOf(m)) + if v.Kind() != reflect.Struct { return nil, errors.New("invalid non struct type") } diff --git a/spdy/copy.go b/spdy/copy.go index cb0aa7e..a946827 100644 --- a/spdy/copy.go +++ b/spdy/copy.go @@ -1,6 +1,7 @@ package spdy import ( + "encoding" "errors" "io" "net" @@ -55,6 +56,13 @@ func (c *channel) copyValue(v interface{}) (interface{}, error) { return c.copyChannelMessage(val) case map[interface{}]interface{}: return c.copyChannelInterfaceMessage(val) + case encoding.BinaryMarshaler: + p, err := val.MarshalBinary() + if err != nil { + return nil, err + } + + return c.copyValue(p) default: if rv := reflect.ValueOf(v); rv.Kind() == reflect.Ptr { if rv.Elem().Kind() == reflect.Struct { @@ -163,10 +171,8 @@ func (c *channel) copyChannelInterfaceMessage(m map[interface{}]interface{}) (in } func (c *channel) copyStructure(m interface{}) (interface{}, error) { - v := reflect.ValueOf(m) - if v.Kind() == reflect.Ptr { - v = v.Elem() - } + v := reflect.Indirect(reflect.ValueOf(m)) + if v.Kind() != reflect.Struct { return nil, errors.New("invalid non struct type") } @@ -177,6 +183,9 @@ func (c *channel) copyStructValue(v reflect.Value) (interface{}, error) { mCopy := make(map[string]interface{}) t := v.Type() for i := 0; i < v.NumField(); i++ { + // TODO(stevvooe): Calling Interface without checking if a type can be + // interfaced may lead to panics. This value copier may need to be + // refactored to handle arbitrary types. vCopy, vErr := c.copyValue(v.Field(i).Interface()) if vErr != nil { return nil, vErr diff --git a/spdy/copy_test.go b/spdy/copy_test.go new file mode 100644 index 0000000..6d29271 --- /dev/null +++ b/spdy/copy_test.go @@ -0,0 +1,38 @@ +package spdy + +import ( + "reflect" + "testing" + "time" +) + +func TestTypeTransmission(t *testing.T) { + + // Add problem types to this type definition. For now, we just care about + // time.Time. + type A struct { + T time.Time + } + + expected := A{T: time.Now()} + + sender, receiver, err := Pipe() + if err != nil { + t.Fatalf("error creating pipe: %v", err) + } + + go func() { + if err := sender.Send(expected); err != nil { + t.Fatalf("unexpected error sending: %v", err) + } + }() + + var received A + if err := receiver.Receive(&received); err != nil { + t.Fatalf("unexpected error receiving: %v", err) + } + + if !reflect.DeepEqual(received, expected) { + t.Fatalf("expected structs to be equal: %#v != %#v", received, expected) + } +} From 41b76d7bc17a5fe7758fd569d7d28a8a16970a4b Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 3 Dec 2014 10:21:17 -0800 Subject: [PATCH 2/2] Move type transmission checks to common package --- copy_test.go | 31 --------------------- spdy/copy_test.go | 28 ++----------------- testutil/check.go | 62 ++++++++++++++++++++++++++++++++++++++++++ testutil/check_test.go | 14 ++++++++++ 4 files changed, 79 insertions(+), 56 deletions(-) create mode 100644 testutil/check.go create mode 100644 testutil/check_test.go diff --git a/copy_test.go b/copy_test.go index 17308a1..78d204e 100644 --- a/copy_test.go +++ b/copy_test.go @@ -4,7 +4,6 @@ import ( "io" "net" "os" - "reflect" "runtime/pprof" "testing" "time" @@ -130,36 +129,6 @@ func TestByteStreamProxy(t *testing.T) { SpawnProxyTest(t, client, server, 1) } -func TestTypeTransmission(t *testing.T) { - // TODO(stevvooe): Ensure that libchan transports can all have this same - // test run against it. - - // Add problem types to this type definition. For now, we just care about - // time.Time. - type A struct { - T time.Time - } - - expected := A{T: time.Now()} - - receiver, sender := Pipe() - - go func() { - if err := sender.Send(expected); err != nil { - t.Fatalf("unexpected error sending: %v", err) - } - }() - - var received A - if err := receiver.Receive(&received); err != nil { - t.Fatalf("unexpected error receiving: %v", err) - } - - if !reflect.DeepEqual(received, expected) { - t.Fatalf("expected structs to be equal: %#v != %#v", received, expected) - } -} - func SpawnProxyTest(t *testing.T, client SendTestRoutine, server ReceiveTestRoutine, proxyCount int) { endClient := make(chan bool) endServer := make(chan bool) diff --git a/spdy/copy_test.go b/spdy/copy_test.go index 6d29271..82f9935 100644 --- a/spdy/copy_test.go +++ b/spdy/copy_test.go @@ -1,38 +1,16 @@ package spdy import ( - "reflect" "testing" - "time" + + "github.com/docker/libchan/testutil" ) func TestTypeTransmission(t *testing.T) { - - // Add problem types to this type definition. For now, we just care about - // time.Time. - type A struct { - T time.Time - } - - expected := A{T: time.Now()} - sender, receiver, err := Pipe() if err != nil { t.Fatalf("error creating pipe: %v", err) } - go func() { - if err := sender.Send(expected); err != nil { - t.Fatalf("unexpected error sending: %v", err) - } - }() - - var received A - if err := receiver.Receive(&received); err != nil { - t.Fatalf("unexpected error receiving: %v", err) - } - - if !reflect.DeepEqual(received, expected) { - t.Fatalf("expected structs to be equal: %#v != %#v", received, expected) - } + testutil.CheckTypeTransmission(t, receiver, sender) } diff --git a/testutil/check.go b/testutil/check.go new file mode 100644 index 0000000..efff948 --- /dev/null +++ b/testutil/check.go @@ -0,0 +1,62 @@ +// Package testutil contains checks that implementations of libchan transports +// can use to check compliance. For now, this will only work with Go, but +// cross-language tests could be added here, as well. +package testutil + +import ( + "io" + "io/ioutil" + "reflect" + "strings" + "testing" + "time" + + "github.com/docker/libchan" +) + +func CheckTypeTransmission(t *testing.T, receiver libchan.Receiver, sender libchan.Sender) { + // Add types that should be transmitted by value to this struct. Their + // equality will be tested with reflect.DeepEquals. + type ValueTypes struct { + I int + T time.Time + } + + // Add other types, that may include readers or stateful items. + type A struct { + // TODO(stevvooe): Ideally, this would be embedded but libchan doesn't + // seem to transmit embedded structs correctly. + V ValueTypes + Reader io.ReadCloser // TODO(stevvooe): Only io.ReadCloser is support for now. + } + + readerContent := "asdf" + expected := A{ + V: ValueTypes{ + I: 1234, + T: time.Now(), + }, + Reader: ioutil.NopCloser(strings.NewReader(readerContent)), + } + + go func() { + if err := sender.Send(expected); err != nil { + t.Fatalf("unexpected error sending: %v", err) + } + }() + + var received A + if err := receiver.Receive(&received); err != nil { + t.Fatalf("unexpected error receiving: %v", err) + } + + if !reflect.DeepEqual(received.V, expected.V) { + t.Fatalf("expected structs to be equal: %#v != %#v", received.V, expected.V) + } + + receivedContent, _ := ioutil.ReadAll(received.Reader) + if string(receivedContent) != readerContent { + t.Fatalf("reader transmitted different content %q != %q", receivedContent, readerContent) + } + +} diff --git a/testutil/check_test.go b/testutil/check_test.go new file mode 100644 index 0000000..085cc85 --- /dev/null +++ b/testutil/check_test.go @@ -0,0 +1,14 @@ +package testutil + +import ( + "github.com/docker/libchan" + + "testing" +) + +// TestTypeTransmission tests the main package (to avoid import cycles) and +// provides and example of how this test should be used in other packages. +func TestTypeTransmission(t *testing.T) { + receiver, sender := libchan.Pipe() + CheckTypeTransmission(t, receiver, sender) +}