Repository: creachadair/jrpc2 (go)

Issue:
`PushCall` fails due to `duplicate request id`
The `jrpc2.PushCall` results in `[-32600] duplicate request id "1"` error being returned to the client side after sending reply from the callback. It seems to be coming from `checkAndAssign` function on the server side.

Attached (simplified and contrived) client-server setup to reproduce the error. 
[client-server.zip](https://github.com/creachadair/jrpc2/files/5919540/client-server.zip)

At first glance, it seems to happen because the check for duplicate ID in `checkAndAssign` function happens before the `PushCall` response it recognized as such and handled:
https://github.com/creachadair/jrpc2/blob/default/server.go#L273-L274
https://github.com/creachadair/jrpc2/blob/default/server.go#L277-L282

I've made a quick fix by moving that section before the duplicate request check and it seems to fix the problem, but I don't know what other impact it might have:
https://github.com/Lachann/jrpc2/commit/3ffe8ef8ad9d7350e94eddf4fb7a67e3cbd153a7




Pull request description:
Fix request validation checks for push calls.
Prior to this change, the server checked the ID of an inbound request before
checking whether it is a callback response. This could result in a spurious
error if the callback happened to use the same ID as an in-flight request.

Update the tests to ensure they tickle this case, and fix the order of checks
so that we do not check for duplicate IDs until AFTER filtering matches of a
push response.

Fixes #36.

Patch:
diff --git a/server.go b/server.go
index c7cf8ba..9864419 100644
--- a/server.go
+++ b/server.go
@@ -268,18 +268,22 @@ func (s *Server) checkAndAssign(next jmessages) tasks {
 			hreq:  &Request{id: fid, method: req.M, params: req.P},
 			batch: req.batch,
 		}
+		id := string(fid)
 		if req.err != nil {
 			t.err = req.err // deferred validation error
-		} else if id := string(fid); id != "" && s.used[id] != nil {
-			t.err = Errorf(code.InvalidRequest, "duplicate request id %q", id)
-		} else if !s.versionOK(req.V) {
-			t.err = ErrInvalidVersion
 		} else if !req.isRequestOrNotification() && s.call[id] != nil {
 			// This is a result or error for a pending push-call.
+			//
+			// N.B. It is important to check for this before checking for
+			// duplicate request IDs, since the ID spaces could overlap.
 			rsp := s.call[id]
 			delete(s.call, id)
 			rsp.ch <- req
 			continue // don't send a reply for this
+		} else if id != "" && s.used[id] != nil {
+			t.err = Errorf(code.InvalidRequest, "duplicate request id %q", id)
+		} else if !s.versionOK(req.V) {
+			t.err = ErrInvalidVersion
 		} else if req.M == "" {
 			t.err = Errorf(code.InvalidRequest, "empty method name")
 		} else if s.setContext(t, id) {
@@ -426,6 +430,7 @@ func (s *Server) pushReq(ctx context.Context, wantID bool, method string, params
 		kind = "call"
 		id := strconv.FormatInt(s.callID, 10)
 		s.callID++
+
 		jid = json.RawMessage(id)
 		rsp = &Response{
 			ch:     make(chan *jmessage, 1),


Test patch:
diff --git a/jrpc2_test.go b/jrpc2_test.go
index e655c27..39967cc 100644
--- a/jrpc2_test.go
+++ b/jrpc2_test.go
@@ -743,17 +743,16 @@ func TestPushCall(t *testing.T) {
 		},
 	})
 	defer loc.Close()
-	s, c := loc.Server, loc.Client
 	ctx := context.Background()
 
-	// Post an explicit callback.
-	if _, err := s.Callback(ctx, "succeed", nil); err != nil {
-		t.Errorf("Callback explicit: unexpected error: %v", err)
+	// Call the method that posts a callback.
+	if _, err := loc.Client.Call(ctx, "CallMeMaybe", nil); err != nil {
+		t.Fatalf("Call CallMeMaybe: unexpected error: %v", err)
 	}
 
-	// Call the method that posts a callback.
-	if _, err := c.Call(ctx, "CallMeMaybe", nil); err != nil {
-		t.Errorf("Call CallMeMaybe: unexpected error: %v", err)
+	// Post an explicit callback.
+	if _, err := loc.Server.Callback(ctx, "succeed", nil); err != nil {
+		t.Errorf("Callback explicit: unexpected error: %v", err)
 	}
 }