[Alexandre Kohler (kwy404)]

Closes #1757

vscode-jsonrpc users can now change or observe the messages a connection sends, not only the ones it receives. The issue needs this to propagate W3C trace context (traceparent) to a StreamJsonRpc process: one side adds a top-level property to each outgoing message, and the other side reads it before the handler runs. Today MessageStrategy only sees received messages, and the writer used by createMessageConnection can't be wrapped without reimplementing the framing.

Design

This follows the second idea in Dirk Bäumer (@dbaeumer)'s comment: keep MessageStrategy and add two methods to it.

• handleOutgoingMessage(message, next) gets every message the connection writes (requests, notifications and responses) before it reaches the MessageWriter. It calls next with the message to write, so it can add properties, replace the message or run code around the write.
• handleIncomingMessage(message, next) gets every received message before it is dispatched. If provided, it is used instead of handleMessage.
• handleMessage keeps its current meaning (received messages only), so existing strategies behave exactly as before. It is now optional, so a strategy can implement only the new methods, and MessageStrategy.is accepts a strategy with any of the three.

All writes in the connection go through one writeMessage helper, which is a plain messageWriter.write when no outgoing hook is set. I did not add a direction parameter to handleMessage, since calling existing strategies for outgoing messages would change what they see.

Example

const connection = createMessageConnection(reader, writer, undefined, {
	messageStrategy: {
		handleOutgoingMessage(message, next) {
			return next({ ...message, traceparent: getTraceparent() } as Message);
		},
		handleIncomingMessage(message, next) {
			const traceparent = (message as { traceparent?: string }).traceparent;
			return runWithTraceparent(traceparent, () => next(message));
		}
	}
});

Tests and docs

• Eight tests in jsonrpc/src/node/test/connection.test.ts: a client adds traceparent to its requests and the server reads it in handleIncomingMessage and exposes it to the request handler through AsyncLocalStorage; a server sees its outgoing notification and response in order; handleIncomingMessage is used instead of handleMessage; an outgoing hook that awaits keeps the send order; with maxParallelism set, a response and the cancellations of a running and a queued request reach handleIncomingMessage; a hook that never calls next on a queued cancellation leaves the request to run and be answered; a throwing hook on a message that skips the queue is logged instead of escaping.
• A short section with an example in jsonrpc/README.md.
• Checks run: npm run compile:jsonrpc, the JSON-RPC Node.js tests and npm run lint in jsonrpc. The browser tests were not run locally.

[Copilot]

🟡 Changes recommended

Incoming fast paths bypass interception, and asynchronous outgoing hooks can reverse message order.

2 open issues

• Preserve message order across asynchronous outgoing hooks · New
• Route fast-path messages through the all-received-messages hook · New

What changed in this PR

Extends vscode-jsonrpc message strategies to support outgoing metadata injection and incoming interception for trace-context propagation in #1757.

Changes:

• Adds optional incoming and outgoing hooks while retaining the legacy incoming hook as a fallback.
• Routes connection writes through a shared hook-aware helper.
• Adds tracing, outgoing-message, and hook-precedence tests plus a usage example.

File	Description
jsonrpc/​src/​node/​test/​connection.test.ts	Tests trace propagation, outgoing messages, and incoming-hook precedence.
jsonrpc/​src/​common/​connection.ts	Extends MessageStrategy and integrates hooks into dispatch and writes.
jsonrpc/​README.md	Documents the hooks with a trace-context example.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Fast-path hook handling can leave requests unanswered or let synchronous exceptions escape IPC event handling.

2 open issues

• Catch and log synchronous handler exceptions · New
• Defer cancellation until request dispatch is confirmed · New

2 resolved since last review

• Preserve message order across asynchronous outgoing hooks
• Route fast-path messages through the all-received-messages hook

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Cancellation handling can take effect before interception or ignore the replacement message passed to the hook’s continuation.

1 open issue

• Apply cancellation bookkeeping only after hook continuation · New

1 previously missed issue

In code that hasn't changed since last review

Honor forwarded cancellation messages after hook interception

jsonrpc/​src/​common/​connection.ts:851

This continuation ignores the message passed to next, so a hook that forwards a replacement cancellation targeting another request still cancels and answers the original request. The running-request path at lines 870–873 likewise cancels the originally captured token. Dispatch the forwarded message and resolve its cancellation target and response after interception, retaining the queued-versus-running recheck for delayed continuations.

2 resolved since last review

• Catch and log synchronous handler exceptions
• Defer cancellation until request dispatch is confirmed

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

There are no active review issues for the message-hook changes supporting trace-context propagation.

No open issues 🎉

1 resolved since last review

• Apply cancellation bookkeeping only after hook continuation

_{Review effort: Balanced}

[Dirk Bäumer (dbaeumer)]

Thanks, this matches the direction we discussed in #1757, and the cancellation and ordering edge cases are well handled. My inline comments are mostly about what happens when a hook does not call next, plus some API and documentation follow-ups.

Could you also run the browser tests, or confirm that CI covers them? connection.ts is shared code.

[Dirk Bäumer (dbaeumer)]

Two things about handleMessage:

1. Making it optional breaks typing for external code that calls strategy.handleMessage(...) directly when strictNullChecks is on. If that is acceptable for this release, please add a note to the changelog. Otherwise we could leave MessageStrategy as it is and add the new methods in a new interface that extends it.
2. handleMessage and handleIncomingMessage now mean almost the same thing; the only difference is whether a message that skips the queue is passed in. Could we mark handleMessage as @deprecated in favor of handleIncomingMessage? Then the API would have one incoming hook and one outgoing hook.

[Alexandre Kohler (kwy404)]

Done in 8575cc1. handleMessage is now marked @deprecated in favor of handleIncomingMessage. I kept it optional and added a note under Compatibility in the history of the root README, in a new Next section. If you would rather avoid the typing change in this release, I can move the new methods to a separate interface.

[Dirk Bäumer (dbaeumer)]

Since cancellations now depend on the hook, a hook that only forwards some messages and forgets $/cancelRequest quietly breaks cancellation. Please state this more directly here (something like "you must call next for $/cancelRequest, or the request is not canceled") and in the README section too.

[Alexandre Kohler (kwy404)]

Fixed in 8575cc1. The JSDoc now says "You must call next for $/cancelRequest, or the request is not canceled", and the README says the same.

[Dirk Bäumer (dbaeumer)]

Adding a property means casting to Message (see the README example: { ...message, traceparent } as Message). Could we make this generic, or at least document that extra top-level properties reach the writer unchanged?

[Alexandre Kohler (kwy404)]

Documented in 8575cc1, in the JSDoc and the README: top level properties reach the writer unchanged, and Message does not declare them, so the object passed to next needs the cast. I did not make it generic, since that would have to go through MessageStrategy and the connection options. Happy to look at it if you prefer that.

[Dirk Bäumer (dbaeumer)]

Nit: the check "does the strategy have handleIncomingMessage?" is repeated here, in triggerMessageQueue and in callback. A small incomingHook() helper would remove the duplication.

[Alexandre Kohler (kwy404)]

Fixed in 8575cc1. incomingHook() now holds that check for all three call sites.

[Dirk Bäumer (dbaeumer)]

If handleOutgoingMessage resolves without calling next, the message is never written but writeMessage still resolves. For a request, sendRequest registers the response promise and waits forever. Calling next twice writes the message twice.

Could we wrap next so it only works once and check whether it was called after the hook settles? If it wasn't, we should reject the pending request (or at least log a warning). If dropping messages is meant to be allowed, the JSDoc should say what happens to requests.

Separately: messages still in this chain can reach messageWriter.write after the connection is disposed. Checking isDisposed() inside the chained callback would prevent that cheaply.

[Alexandre Kohler (kwy404)]

Fixed in 8575cc1. next now only works once: any other call, including one after the hook settled, logs a warning and writes nothing. If the hook settles without calling next, writeMessage rejects, so sendRequest rejects the pending request with MessageWriteError through its existing catch, and the promise of a notification rejects too. The JSDoc and README describe this. The chained callback also checks isDisposed() first, so a queued message does not reach the hook or the writer after dispose. New tests cover all of these cases.

[Dirk Bäumer (dbaeumer)]

This should keep the chain working after a hook throws or rejects, but no test checks it. Please add one where the hook fails on one message and the next message is still written.

[Alexandre Kohler (kwy404)]

Added in 8575cc1. The test runs twice, with a hook that throws and with one that rejects, and checks that the failed send rejects and the next message is still written. It also passes without the other changes in that commit, so it guards the existing behavior.

[Dirk Bäumer (dbaeumer)]

Please add that a handleIncomingMessage hook must call next for $/cancelRequest, or cancellation stops working. Also mention what happens if handleOutgoingMessage doesn't call next.

[Alexandre Kohler (kwy404)]

Added in 8575cc1, both the $/cancelRequest requirement and what happens if handleOutgoingMessage does not call next.

[Alexandre Kohler (kwy404)]

All the inline comments are addressed in 8575cc1. I ran npm run test:node and npm run test:browser in jsonrpc on Windows and both pass, and .github/workflows/build.yml covers the browser tests through npm run test. jsonrpc, server and client compile, and eslint reports nothing on the two changed source files. I did not run the client integration tests.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because subtle interactions between asynchronous hooks and writer promises can mask transport failures and leave requests pending.

No open issues 🎉

_{Review effort: Balanced}

[Alexandre Kohler (kwy404)]

One more in fbf0378: writeMessage now also waits for the promise of next. A hook that called next without returning it lost a failed write, so the request waited forever. The new test fails without this change. npm run test:node and npm run test:browser in jsonrpc pass.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because asynchronous hook handling can leave writer rejections unhandled, potentially terminating Node.js even when callers catch the send failure.

No open issues 🎉

_{Review effort: Balanced}

[Alexandre Kohler (kwy404)]

One more in 8a7946e: a failed write no longer becomes an unhandled rejection while a hook that did not await next is still running. The failure is still reported once the hook settles. The new test fails without this change. npm run test:node and npm run test:browser in jsonrpc pass.

[Copilot]

🔵 Needs a closer look

Human assessment is needed because unpublished findings identify message-hook lifecycle and ordering risks, including writes after end or disposal and skipped hooks for canceled requests.

No open issues 🎉

_{Review effort: Balanced}