Skip to content

Commit

Permalink
Add call validation, and fix flaky call validation tests
Browse files Browse the repository at this point in the history
Previously, we only validated that a timeout was set, not that a
servicename was specified when making an outbound call.

Add validation, and do it before we get a connection. This avoids
creating a connection that will not be used.

Previously, we would create a connection, do validation, and close the
connection immediately. On the inbound side, this led to a new
connection that was closed almost immediately, and so sometimes the test
would cause a:
"Failed to add connection to peer" "connection is in an invalid state"

By doing validation before we create a connection we make TestNoTimeout
less flaky, while also returning errors earlier and faster.
  • Loading branch information
prashantv committed Jun 17, 2016
1 parent 0cedce3 commit 061aad9
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 6 deletions.
13 changes: 12 additions & 1 deletion connection_test.go
Expand Up @@ -314,13 +314,24 @@ func TestNoTimeout(t *testing.T) {

ctx := context.Background()
_, _, _, err := raw.Call(ctx, ts.Server(), ts.HostPort(), "svc", "Echo", []byte("Headers"), []byte("Body"))
require.NotNil(t, err)
assert.Equal(t, ErrTimeoutRequired, err)

ts.AssertRelayStats(relay.NewMockStats())
})
}

func TestNoServiceNaming(t *testing.T) {
testutils.WithTestServer(t, nil, func(ts *testutils.TestServer) {
ctx, cancel := NewContext(time.Second)
defer cancel()

_, _, _, err := raw.Call(ctx, ts.Server(), ts.HostPort(), "", "Echo", []byte("Headers"), []byte("Body"))
assert.Equal(t, ErrNoServiceName, err)

ts.AssertRelayStats(relay.NewMockStats())
})
}

func TestServerBusy(t *testing.T) {
testutils.WithTestServer(t, nil, func(ts *testutils.TestServer) {
ts.Register(ErrorHandlerFunc(func(ctx context.Context, call *InboundCall) error {
Expand Down
23 changes: 18 additions & 5 deletions outbound.go
Expand Up @@ -47,8 +47,9 @@ func (c *Connection) beginCall(ctx context.Context, serviceName, methodName stri
}

deadline, ok := ctx.Deadline()
// No deadline was set, we should not support no deadlines.
if !ok {
// This case is handled by validateCall, so we should
// never get here.
return nil, ErrTimeoutRequired
}
timeToLive := deadline.Sub(now)
Expand Down Expand Up @@ -214,10 +215,6 @@ func (call *OutboundCall) createStatsTags(connectionTags map[string]string, call

// writeMethod writes the method (arg1) to the call
func (call *OutboundCall) writeMethod(method []byte) error {
if len(method) > maxMethodSize {
return call.failed(ErrMethodTooLarge)
}

call.statsReporter.IncCounter("outbound.calls.send", call.commonStatsTags, 1)
return NewArgWriter(call.arg1Writer()).Write(method)
}
Expand Down Expand Up @@ -369,3 +366,19 @@ func (response *OutboundCallResponse) doneReading(unexpected error) {

response.mex.shutdown()
}

func validateCall(ctx context.Context, serviceName, methodName string, callOpts *CallOptions) error {
if serviceName == "" {
return ErrNoServiceName
}

if len(methodName) > maxMethodSize {
return ErrMethodTooLarge
}

if _, ok := ctx.Deadline(); !ok {
return ErrTimeoutRequired
}

return nil
}
4 changes: 4 additions & 0 deletions peer.go
Expand Up @@ -455,6 +455,10 @@ func (p *Peer) BeginCall(ctx context.Context, serviceName, methodName string, ca
}
callOptions.RequestState.AddSelectedPeer(p.HostPort())

if err := validateCall(ctx, serviceName, methodName, callOptions); err != nil {
return nil, err
}

conn, err := p.GetConnection(ctx)
if err != nil {
return nil, err
Expand Down

0 comments on commit 061aad9

Please sign in to comment.