Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interceptors are called for every request. #1436

Open
simoncave opened this issue Feb 27, 2025 · 0 comments
Open

Interceptors are called for every request. #1436

simoncave opened this issue Feb 27, 2025 · 0 comments
Labels
bug Something isn't working

Comments

@simoncave
Copy link

Describe the bug

  • Expected behavior: Interceptors are called during transport creation, and the resulting handler is reused for every request.
  • Actual Behaviour: Interceptors are called for every request.

The consequence of this behaviour is that it prevents interceptors from operating across multiple requests in a safe way. See the additional context section for an example.

To Reproduce

let numInterceptorCalls = 0;
let numHandlerCalls = 0;

const interceptor: Interceptor = (next) => {
  numInterceptorCalls++;
  return async (request) => {
    numHandlerCalls++;
    return next(request);
  };
};

const transport = createRouterTransport(
  (router) => {
    router.service(GreetingService, {
      greet: ({ name }) => ({ greeting: `Hello, ${name}!` }),
    });
  },
  {
    transport: {
      interceptors: [interceptor],
    },
  },
);

const client = createClient(GreetingService, transport);

await client.greet({ name: "Sun" });
await client.greet({ name: "Moon" });
await client.greet({ name: "World" });

expect(numHandlerCalls).toBe(3);
expect(numInterceptorCalls).toBe(1); // Actual value: 3

Environment

  • @connectrpc/connect-web version: 2.0.1
  • @connectrpc/connect-node version: 2.0.1
  • Frontend framework and version: react@19, next@15
  • Node.js version: (for example, 23.2.0)
  • Browser and version: (for example, Google Chrome Version 133.0.6943.127 (Official Build) (arm64))

Additional context

Let's say I wanted to create an interceptor which deduplicates identical requests made to idempotent methods using React's cache function. My expectation was that I could create a request cache inside the interceptor, where I have access to the next handler. The request cache would check each request to see if it matched a previous request and, if it does, produce the cached response.

Note

For brevity, this example only checks the method and request message. It ignores headers, context values, and abort signals.

import assert from "node:assert";
import { equals } from "@bufbuild/protobuf";
import { MethodOptions_IdempotencyLevel } from "@bufbuild/protobuf/wkt";
import { Interceptor, UnaryRequest, UnaryResponse } from "@connectrpc/connect";
import { cache } from "react";

export const reactCacheInterceptor: Interceptor = (next) => async (request) => {
  const handler = getCachedHandler(next);
  const response = await handler(request);
  return response;
};

const getCachedHandler = cache<Interceptor>((next) => {
  const cache = new Map<UnaryRequest, Promise<UnaryResponse>>();

  return (request) => {
    const canCache =
      request.method.methodKind === "unary" &&
      [
        MethodOptions_IdempotencyLevel.IDEMPOTENT,
        MethodOptions_IdempotencyLevel.NO_SIDE_EFFECTS,
      ].includes(request.method.idempotency);

    if (!canCache) {
      return next(request);
    }

    // TypeScript doesn't know that the method is unary.
    assert(request.stream === false);

    for (const [cachedRequest, cachedResponsePromise] of cache.entries()) {
      if (
        request.method === cachedRequest.method &&
        equals(request.method.input, request.message, cachedRequest.message)
      ) {
        return cachedResponsePromise;
      }
    }

    const responsePromise = next(request).then((response) => {
      // TypeScript doesn't know that the method is unary.
      assert(response.stream === false);
      return response;
    });

    cache.set(request, responsePromise);

    return responsePromise;
  };
});

Unfortunately, this doesn't work. As the interceptor is called for each request, the request cache is created for each request too. As the next argument is also unique for each request, storing caches in a WeakMap and looking them up inside the handler doesn't work either.

There are a few possible workarounds:

  1. Use a global request cache. This is unsafe. If the same interceptor is used in multiple transports, then responses can leak between transports.
  2. Create a custom transport that calls interceptors once.
  3. Create a custom client that dedupes requests before sending them to the transport.
@simoncave simoncave added the bug Something isn't working label Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant