Update HandlerContext signal to trigger on other reasons than timeouts

[ghost]

This PR renames the AbortSignal property in the HandlerContext from deadline to a more generic signal.

It also adds a signal to universal server requests and links it to the handler context signal.

We expect server plugins to trigger the AbortSignal of the request when a request is finished or aborted prematurely.

As a result, service implementations can now rely on the signal of the HandlerContext to clean up, whether the RPC timed out (via a deadline), whether the client canceled the request, or whether the request finished normally.

[timostamm]

Thank you for the PR, Johyn! We'll take a look soon.

[CLAassistant]

All committers have signed the CLA.

[ghost]

After reading #591, I wonder if we could just abort the deadline signal when the request is closed instead of adding another signal.

[timostamm]

Hey @johynpapin, took me a minute to get back to this 😊

Adding the signal to the UniversalServerRequest, aborting it in node-universal-handler.ts, and making it available in the HandlerContext is a good call.

There is one small but important bit though: HTTP/2 has a set of error codes (spec), and gRPC defines a mapping between these codes, and gRPC error codes (spec). If a HTTP/2 client cancels a request, it should close the stream with the HTTP/2 code CANCEL (implemented in #552). The server should pick this code up, and convert it to a Connect/gRPC code "canceled".

If you can spare the time to update the PR, we'd be super happy to have it 🙂

[ghost]

Hey @timostamm! 👋

Thank you very much for this detailed answer. I suspected that there was a problem with the error handling, it's clearer to me now.

I will take the time to understand and make these changes! 😄

[timostamm]

Looks great!

Do not worry about the CI failure. The fork does not have access to the secret that is required for browserstack. As long as make passes (as it did), everything is fine, since we are not touching code that directly affects @bufbuild/connect-web.

I'm going to think about test coverage tomorrow morning and get back to you.

[timostamm]

I pushed up basic tests in cdd0e81. They simply assert that the signal from the request triggers the signal in the HandlerContext. We do not verify that an error raised from this signal is correctly written to the response. We will follow up with test coverage in the test suite that runs a Node.js server.

[timostamm]

Thank for for the contribution, Johyn, LGTM!

I'm going to update the PR description, and also update the description of #591 for the renamed HandlerContext property.

[timostamm]

Thank for for the contribution, Johyn, LGTM!

I'm going to update the PR description, and also update the description of #591 for the renamed HandlerContext property.