encoding/thrift: Add mechanisms to handle inbound requests via thriftrw's 'nowire' implementation

[witriew]

thriftrw's 'nowire' implementation (known as 'streaming' in the thriftrw
context) is an alternative wire-compatible implementation of the Thrift
protocol that bypasses the intermediate 'thriftrw.Wire' representation and
'streams' the protocol directly to the related objects.

This provides the mechanisms to handle 'Call's made via the 'nowire'
implementation instead of the existing 'Wire'-intermediary format. Doing so
involved creating a new type, thriftNoWireHandler, that implements both a
transport.UnaryHandler and a transport.OnewayHandler.

These call into a new interface, NoWireHandler (which will be implemented by
generated code, for each procedure), that understands how to unpack the request
using thriftrw's 'nowire' primitives (stream.RequestReadeer), execute the
request, and return objects that understand what's involved in sending back a
response, if it's a Unary call.

[abhinav]

Some context on the discussion @witriew and I had about this, which, in
hindsight, I should have written down and shared with you, @Dogild and
@AllenLuUber.

Some of the decisions made in encoding/thrift, after a couple years of
hindsight, were not great. For example, one of these is the decision to use
newtype wrappers around funcs (like type UnaryHandler func(..)). This makes
it impossible to have a clean upgrade path for that implementation. For
example, had we used a singular Handler interface, we'd be able to add the
new NoWireHandler interface and internally upgrade from Handler to
NoWireHandler if available with fewer intrusive changes.

When Wit was designing the interfaces here, I suggested that since these were
new APIs largely unencumbered by the burden of backwards comaptibility, he
should err towards choices that we would make today if we were designing these
APIs from scratch.

[Dogild]

Is there any chance to use a mock for NoWireHandler?

[abhinav]

Addendum to the previous comment: For objects with behavioral, mocks can be preferable if the behavior is non-trivial.

Alternatively, a test-only noWireHandlerFunc type that allows test cases to write the behavior inline as needed is also a good option if the assertions aren't shared between the different cases.

type noWireHandlerFunc func(context.Context, *NoWireCall) (NoWireResponse, error)

func (f noWireHandlerFunc) HandleNoWire(ctx context.Context, nwc *NoWireCall) (NoWireResponse, error) {
  return f(ctx, nwc)
}

[witriew]

In the case of the NoWireHandler stub, I wanted to ensure that the stuff thats passed into HandleNoWire is what is expected. Since all of the calls here have effectively the same checks, it seems cleaner to have the stub that does all of this at once instead of repeating these checks for each of the mocks.

[abhinav]

Okay, so this stack isn't green because of:

# go.uber.org/yarpc/encoding/thrift [go.uber.org/yarpc/encoding/thrift.test]
./inbound_nowire_test.go:248:3: cannot use proto (type *streamtest.MockRequestReader) as type stream.Protocol in field value:
        *streamtest.MockRequestReader does not implement stream.Protocol (missing Reader method)

That's a result of thriftrw/thriftrw-go#532 — RequestReaders and Protocols are different interfaces now so the same object cannot be used for them.

Given that this is a new code path, I don't see value in doing upgrade-protocol-to-requestreader dance and just having it be RequestReader-only: All stream based implementations must implement RequestReader. Going to update the PR stack with fixes.

[codecov]

Codecov Report

Merging #2084 (68ba5cd) into nowiredev (e76e1ed) will increase coverage by 0.03%.
The diff coverage is 91.48%.

@@              Coverage Diff              @@
##           nowiredev    #2084      +/-   ##
=============================================
+ Coverage      87.58%   87.61%   +0.03%     
=============================================
  Files            247      248       +1     
  Lines          13731    13772      +41     
=============================================
+ Hits           12026    12067      +41     
  Misses          1325     1325              
  Partials         380      380              

Impacted Files	Coverage Δ
encoding/thrift/thriftrw-plugin-yarpc/server.go	100.00% <ø> (ø)
encoding/thrift/inbound_nowire.go	90.24% <90.24%> (ø)
encoding/thrift/inbound.go	93.02% <100.00%> (ø)
encoding/thrift/outbound.go	84.61% <100.00%> (ø)
encoding/thrift/register.go	83.78% <100.00%> (ø)
serialize/serialize.go	76.47% <100.00%> (ø)
peer/hashring32/internal/hashring32/hashring32.go	97.29% <0.00%> (+1.08%)	⬆️
transport/http/peer.go	97.14% <0.00%> (+1.90%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e76e1ed...68ba5cd. Read the comment docs.

[abhinav]

LGTM besides nits.

[abhinav]

Addendum to the previous comment: For objects with behavioral, mocks can be preferable if the behavior is non-trivial.

Alternatively, a test-only noWireHandlerFunc type that allows test cases to write the behavior inline as needed is also a good option if the assertions aren't shared between the different cases.

type noWireHandlerFunc func(context.Context, *NoWireCall) (NoWireResponse, error)

func (f noWireHandlerFunc) HandleNoWire(ctx context.Context, nwc *NoWireCall) (NoWireResponse, error) {
  return f(ctx, nwc)
}