Review comments

yarpc · Aug 30, 2021 · 40fc262 · 40fc262
1 parent 039ecc8
commit 40fc262
Show file tree

Hide file tree

Showing 2 changed files with 9 additions and 6 deletions.
diff --git a/encoding/thrift/options.go b/encoding/thrift/options.go
@@ -158,9 +158,10 @@ func Protocol(p protocol.Protocol) Option {
 }
 
 // NoWire is an option that specifies to *not* use the thriftrw.Wire
-// intermediary format.  Note that if this is enabled, and a
-// 'protocol.Protocol' is provided (e.g., via thrift.Protocol)m that provided
-// protocol must be able to satisfy the 'stream.Protocol' interface.
+// intermediary format. Note that if this is enabled, and a
+// 'protocol.Protocol' is provided (e.g., via thrift.Protocol) that provided
+// protocol must be able to satisfy the 'stream.RequestReader' interface.
+// Will become the default option.
 func NoWire(enable bool) Option {
 	return noWireOption{Enable: enable}
 }

diff --git a/encoding/thrift/register.go b/encoding/thrift/register.go
@@ -95,8 +95,10 @@ func BuildProcedures(s Service, opts ...RegisterOption) []transport.Procedure {
 		proto = rc.Protocol
 	}
 
+	// Only if we're trying to use the 'NoWire' implementation do we check if the
+	// config's `Protocol` provides the streaming ones needed.
 	var streamReqReader stream.RequestReader = binary.Default
-	if rc.Protocol != nil && rc.NoWire {
+	if rc.NoWire && rc.Protocol != nil {
 		if sp, ok := rc.Protocol.(stream.RequestReader); ok {
 			streamReqReader = sp
 		}
@@ -115,7 +117,7 @@ func BuildProcedures(s Service, opts ...RegisterOption) []transport.Procedure {
 		case transport.Unary:
 			if rc.NoWire {
 				spec = transport.NewUnaryHandlerSpec(thriftNoWireHandler{
-					NoWireHandler: method.HandlerSpec.NoWire,
+					Handler:       method.HandlerSpec.NoWire,
 					RequestReader: streamReqReader,
 				})
 			} else {
@@ -128,7 +130,7 @@ func BuildProcedures(s Service, opts ...RegisterOption) []transport.Procedure {
 		case transport.Oneway:
 			if rc.NoWire {
 				spec = transport.NewOnewayHandlerSpec(thriftNoWireHandler{
-					NoWireHandler: method.HandlerSpec.NoWire,
+					Handler:       method.HandlerSpec.NoWire,
 					RequestReader: streamReqReader,
 				})
 			} else {