Avoid incorrect maintained conn state

[mustard-mh]

This PR fixes the issue where the connection state maintained in lib was not identical to http2, resulting in no new connections being created. see also #683 (comment)

We use conn.destroyed and conn.closed since those properties are exposed in http2 core, and where they throw The session has been destroyed ¹ are all checking with those properties.

And properties are compatible [http2/core v16.0] [connect-node min support v16.0]

Related Issues

Fix #680, #683

How to test

No idea, but we released a patch fork in npm and use it in our product [1], errors are gone

Footnotes

1. search ERR_HTTP2_INVALID_SESSION in http2/core, they all check if (this.destroyed) { throw } ↩

[CLAassistant]

All committers have signed the CLA.

[akosyakov]

@timostamm I can confirm no more GOAWAY and session has been destroyed errors. Our SLO went up from 90 to 97%. We were running with this change for last 5 days.

[timostamm]

Hey @mustard-mh, thank you for the PR. I'm glad to hear the patch brought down your error numbers, but I hope you understand we're hesitant to merge without understanding what is actually happening.

[timostamm]

This will avoid the problem found in #683 (comment). I think we can simply put all connections that received any GOAWAY frame into the "shutting down" state. We will need a test.

[timostamm]

Pushed up a test in bf4d746. New requests open a new connection 👍

But the test framework reports an uncaught exception, see run log. That's because we remove event listeners for the connection here: https://github.com/bufbuild/connect-es/blob/4d1580fe8769a07be4aeb25f2eb4a411e90861f0/packages/connect-node/src/http2-session-manager.ts#L634-L644

Pushed up a15680d to use isShuttingDown for all GOAWAY frames. This means we are deferring the cleanup in onExitState, and pass the test without uncaught exception.

[timostamm]

@mustard-mh, pushed up a test and a change that moves the logic into isShuttingDown because to not leave uncaught errors. That way, you're still credited for your contribution.

[timostamm]

CI failure is just browserstack, which does not have credentials in the fork. Since this does not affect any code tested in browserstack, we're good.

[mustard-mh]

Thank you @timostamm 🙏 , we will try new release on our side

[akosyakov]

@timostamm We tried and unfortunately had to revert back to our fork, i.e. original @mustard-mh changes. We still see The session has been destroyed internal errors.