Express Connect Middleware does not call next() for handled requests

[easyCZ]

Describe the bug

In the implementation of the express connect middleware, the middleware only calls next() when it does not find a corresponding registered connect handler.

However, it should also call next() after it finds an appropriate handler, and handles the request. This then allows any registered middlewares afterwards to also run. This is useful for any number of use-cases, such as logging, telemetry, etc and in the absence of

• Add interceptor capabilities to ConnectRouter #527

it is the only way to actually get some telemetry out after the request has been handled.

To Reproduce

app.use(
      (req, res, next) => {
          	console.log("pre middleware");
			next();
      },
      expressConnectMiddleware({ routes }),
      (req, res, next) => {
          	log.info("post middleware");
			// this is never executed
      },
  );

If you encountered an error message, please copy and paste it verbatim.
If the bug is specific to an RPC or payload, please provide a reduced
example.

Environment (please complete the following information):

• @bufbuild/connect-web version: latest
• @bufbuild/connect-node version: latest

[timostamm]

Thanks for the issue, Milan, we'll get this fixed.

[easyCZ]

@timostamm I can try to put in a PR, if you'd like. I am, however, not that proficient with express to judge if what I'm asking is indeed the correct pattern to go for.

[smaye81]

Hi @easyCZ, in looking at this and Express documentation, I think our middleware is behaving according to Express semantics. If you look at the docs here (4th section), it says:

If the current middleware function does not end the request-response cycle, it must call next() to pass control to the next middleware function. Otherwise, the request will be left hanging.

Our middleware does end the request-response cycle (unless a route isn't found for a specific request) so I think it's working correctly here. I understand what you're asking though. What about using a basic middleware function in addition to ours that added an event listener to the close event of the response? So something like:

app.use((req, res, next) => {
    console.log("got request", req.url);
    res.on("close", () => {
        console.log("emit metrics here", req.url);
    });
    next();
})

[smaye81]

@easyCZ going to close this issue but feel free to re-open or create a new one if you notice something still not working correctly.