jsonrpc2: do not panic when sending unmarshalable params

If params given to (*Connection).Call is not JSON-marshalable, Call
returns immediately an *AsyncCall instance containing the
corresponding error, but its ctx field is not initialized. This
produces a panic later in golang.org/x/exp/event.New, as it does not
expect a nil context.Context.

Fixes golang/go#61654.

Change-Id: I6464ab5fde85670267b25821e0b57af04c331c8c
Reviewed-on: https://go-review.googlesource.com/c/exp/+/516556
Reviewed-by: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Michael Knyszek <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>

Author: maxatome

jsonrpc2/conn.go

@@ -144,18 +144,18 @@ func (c *Connection) Call(ctx context.Context, method string, params interface{}
 		id:        Int64ID(atomic.AddInt64(&c.seq, 1)),
 		resultBox: make(chan asyncResult, 1),
 	}
+	// TODO: rewrite this using the new target/prototype stuff
+	ctx = event.Start(ctx, method,
+		Method(method), RPCDirection(Outbound), RPCID(fmt.Sprintf("%q", result.id)))
+	Started.Record(ctx, 1, Method(method))
+	result.ctx = ctx
 	// generate a new request identifier
 	call, err := NewCall(result.id, method, params)
 	if err != nil {
-		//set the result to failed
+		// set the result to failed
 		result.resultBox <- asyncResult{err: errors.Errorf("marshaling call parameters: %w", err)}
 		return result
 	}
-	//TODO: rewrite this using the new target/prototype stuff
-	ctx = event.Start(ctx, method,
-		Method(method), RPCDirection(Outbound), RPCID(fmt.Sprintf("%q", result.id)))
-	Started.Record(ctx, 1, Method(method))
-	result.ctx = ctx
 	// We have to add ourselves to the pending map before we send, otherwise we
 	// are racing the response.
 	// rchan is buffered in case the response arrives without a listener.
@@ -355,7 +355,7 @@ func (c *Connection) manageQueue(ctx context.Context, preempter Preempter, fromR
 			select {
 			case nextReq, ok = <-fromRead:
 			case toDeliver <- q[0]:
-				//TODO: this causes a lot of shuffling, should we use a growing ring buffer? compaction?
+				// TODO: this causes a lot of shuffling, should we use a growing ring buffer? compaction?
 				q = q[1:]
 			}
 		}
@@ -413,7 +413,7 @@ func (c *Connection) reply(entry *incoming, result interface{}, rerr error) {
 	}
 	if err := c.respond(entry, result, rerr); err != nil {
 		// no way to propagate this error
-		//TODO: should we do more than just log it?
+		// TODO: should we do more than just log it?
 		event.Error(entry.baseCtx, "jsonrpc2 message delivery failed", err)
 	}
 }
@@ -441,7 +441,7 @@ func (c *Connection) respond(entry *incoming, result interface{}, rerr error) er
 			err = errors.Errorf("%w: %q notification failed: %v", ErrInternal, entry.request.Method, rerr)
 			rerr = nil
 		case result != nil:
-			//notification produced a response, which is an error
+			// notification produced a response, which is an error
 			err = errors.Errorf("%w: %q produced unwanted response", ErrInternal, entry.request.Method)
 		default:
 			// normal notification finish

jsonrpc2/jsonrpc2_test.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"path"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -60,6 +61,7 @@ var callTests = []invoker{
 		notify{"unblock", "a"},
 		collect{"a", true, false},
 	}},
+	callErr{"error", func() {}, "marshaling call parameters: json: unsupported type"},
 }
 
 type binder struct {
@@ -90,6 +92,12 @@ type call struct {
 	expect interface{}
 }
 
+type callErr struct {
+	method    string
+	params    interface{}
+	expectErr string
+}
+
 type async struct {
 	name   string
 	method string
@@ -175,6 +183,18 @@ func (test call) Invoke(t *testing.T, ctx context.Context, h *handler) {
 	verifyResults(t, test.method, results, test.expect)
 }
 
+func (test callErr) Name() string { return test.method }
+func (test callErr) Invoke(t *testing.T, ctx context.Context, h *handler) {
+	var results interface{}
+	if err := h.conn.Call(ctx, test.method, test.params).Await(ctx, &results); err != nil {
+		if serr := err.Error(); !strings.Contains(serr, test.expectErr) {
+			t.Fatalf("%v:Call failed but with unexpected error: %q does not contain %q", test.method, serr, test.expectErr)
+		}
+		return
+	}
+	t.Fatalf("%v:Call succeeded (%v) but should have failed with error containing %q", test.method, results, test.expectErr)
+}
+
 func (test echo) Invoke(t *testing.T, ctx context.Context, h *handler) {
 	results := newResults(test.expect)
 	if err := h.conn.Call(ctx, "echo", []interface{}{test.method, test.params}).Await(ctx, results); err != nil {