Implement general middleware for requests and notifications

[vinistock]

Closes #918

Allow adding a general middleware provideRequestOrNotification to client options that is used for all requests and notifications. The attached issue has a bit more context, but this is useful for measuring performance and even appending additional parameters to requests if desired.

Please let me know if this is the right direction for the implementation and if I'm following the right style and naming conventions. I wasn't sure if this was the right level for the implementation, but the general middleware can alternatively be implemented as a connection option instead.

[dbaeumer]

@vinistock this looks all good to me. Could you please rebase the PR on latest from main. I needed to change the two methods a little bit due to a serious bug with document synchronization. Thanks Dirk

[vinistock]

@microsoft-github-policy-service agree

[vinistock]

Not sure why the windows build timed out, but I assume it's not related to these changes. Let me know if that's not the case.

[dbaeumer]

Actually thinking about this signature again it is not very client friendly. The reason is that the params array is hard to interpret in that form for a middleware since it can have the following combinatation:

• length 0: no param not token
• length 1: (no param and a token) or (a param no token)
• lenght 2: param and token

I have no good idea yet on how to improve this.

If all we want is to support some logging when sendRequest / sendNotification is invoked we might want to add a different API than a middleware which should allow interprestation / modification.

[dbaeumer]

@vinistock any idea regarding the above comment.

[dbaeumer]

One idea could be to have a signature like

(type: string | MessageSignature, param: any | undefined, token: CancellationToken | undefined) => Promise<R>

and then unfold the params and fold them again in the next handler.

[vinistock]

The main use case for us is for doing performance and error metrics. Something like this

const clientOptions = {
  middleware: {
    sendRequest: () => {
      try {
        // Benchmark running the request (invoke next with params)
      } catch (error) {
        // Report error returned by the server
      }
    }
  }
}

For this scenario, I do think a middleware would make the most sense. It provides a lot of flexibility on what clients can do around requests and notifications.

In terms of a different API, one possibility would be to "break" the middleware into parts. This is not as flexible, and frankly feels less convenient than a middleware, but it might be easier to define an interface for. For example,

const clientOptions = {
  beforeRequest: (type, params) => { /* start measuring performance */ },
  afterRequest: (type, params) => { /* stop measuring performance and log */ },
  onRequestError: (error, type, params) => { /* log error */ } // Could potentially be a part of `afterRequest` to avoid an extra handler
}

and then unfold the params and fold them again in the next handler.

Can you explain with an example? I'm not sure I understand the approach.

[dbaeumer]

Not syntax or semantic checked but here is what I mean:

interface GeneralMiddleware {
	sendRequest?<P, R>(
		this: void,
		type: string | MessageSignature,
		param: P | undefined,
		token: CancellationToken | undefined,
		next: (type: string | MessageSignature, param: P | undefined, token?: CancellationToken) => Promise<R>,
	): Promise<R>;

	sendNotification?<P>(
		this: void,
		type: string | MessageSignature,
		params: P,
		next: (type: string | MessageSignature, params?: P) => Promise<void>
	): Promise<void>;
}

and in sendRequest

		const _sendRequest = this._clientOptions.middleware?.sendRequest;
		if (_sendRequest !== undefined) {
			const param: any | undefined = undefined;
			const token: CancellationToken | undefined = undefined;
			if (params.length === 1) {
				if (CancellationToken.is(params[0])) {
					token = params[0];
				} else {
					param = params[0];
				}
			} else if (params.length === 2) {
				param = params[0];
				token = params[1];
			}
			return _sendRequest(type, param, token, (type, param, token) => {
				const params: any[] = [];
				if (param !== undefined) {
					params.push(param);
				}
				if (token !== undefined) {
					params.push(token);
				}
				return connection.sendRequest<R>(type, ...params);
			});
		} else {
			return connection.sendRequest<R>(type, ...params);
		}

[vinistock]

I added your suggestion to the latest commit, so that it's easier to analyze and drop if necessary.

I wonder if there's a type signature we can come up for params that would improve the client interface without requiring this folding and unfolding of params. Something like

// Params can be a CancellationToken, an `any` set of params, both of those or undefined
type ParamType = [CancellationToken] | [any] | [any, CancellationToken] |  undefined;

interface GeneralMiddleware {
	sendRequest?<R>(
		this: void,
		type: string | MessageSignature,
		next: (type: string | MessageSignature, params: ParamType) => Promise<R>,
		...params: ParamType,
	): Promise<R>;
}

Not sure if TypeScript would be smart enough to differentiate those or if any would just mess it up.

[dbaeumer]

In general we do these checks with TS type check functions. IMO you should be able to simply use: https://insiders.vscode.dev/github/microsoft/vscode-languageserver-node/blob/fca08a7e9f9bd9beeb687a5ae5529139783e915b/jsonrpc/src/common/cancellation.ts#L43

[vinistock]

I imported and used it instead. Let me know if you believe this is indeed the way to go and if we should do the same for notifications.

[dbaeumer]

Protocol reexport vscode-jsonrpc. So you should get that type from vscode-languageserver-protocol

[vinistock]

Done.

[vinistock]

@dbaeumer do we need to do the same separation for notifications as well? Let me know and I can follow up with another PR.

[dbaeumer]

@vinistock what exactly do you mean?

[vinistock]

Sorry, I just realized I got confused. I meant if we needed to do the folding and unfolding of parameters for notifications too, but I don't think notifications have a cancellation token.

[dbaeumer]

Correct, notifications have no token.

[vinistock]

Wanted to report back that I tested this on 8.2.0-next.0 and it's working well for our purposes. Here are the changes in our client if you'd like to see an example usage.

[ejgallego]

That's exactly what we needed for coq-lsp to actually instrument the calls to our server worker, thanks!