fix(node-http-handler): http2 lets node exit

[simonbuchan]

Description

Http2Session keeps node alive even with no requests running, presumably for server pushed streams. This is a behavior change from node's http 1 Agent, which does not keep node alive when there is no request running.

Emulate that behavior for Node2HttpHandler by unref()ing new sessions, and wrapping requests in a ref() / unref() pair.

This issue is mentioned by #1206, but it incorrectly assumes it's strictly related to connection timeouts. The interaction there is a bit strange to describe, since the actual network connection and request establishment overlap, but the behavior you should see is that if for example, you immediately create and abort a request, the session connection continues, but node will exit as the session is no longer ref()ed. Would be good to explicitly test these weird cases, but that's mostly testing node's http2 module, not the SDK code, and I don't really have that set up.

Testing

Tested by npm linking to a project which was directly using NodeHttp2Handler (for context, I am send sigV4 signed requests to API gateway and AppSync using the SDK signing and request machinery without needing to deal with the SDK service middleware configuration - probably not the cleanest option)

Without this, it hangs if any requests are made. With it, cleanly returns after making a couple requests and reading the response body with streamCollector.

I would be happy to add some unit test code as well, but since neither Http2Session nor the Socket it forwards the ref() / unref() calls to have the hasRef() method, I'm not sure what that would look like. I guess spying on the ref() / unref() calls, and checking they are happening at the right time? Let me know what you think. I don't think there's too much room for issues here, given all the success and error paths already always go through the request close event, so so long as the underlying node request stream gets the abort / close / destroy / goaway message it should always work, but not having anything is not great.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[AllanZhengYP]

@simonbuchan Huge thanks for the PR, you cleared the mist. I agree it is pretty hard to test, because we need to validate the programe to exit. So I think it's easier to validate ref() and unref() are called respectively before the request and after the "close" event.

Once this is merged, I can switch Kinesis client to use the H2 handler, and add an integration test with a relatively small jest.timeout().

[simonbuchan]

So I think it's easier to validate ref() and unref() are called respectively before the request and after the "close" event.

Do you mean in this PR? I'm happy to give it a try but I'll take any suggestions. It seemed like it would be a bit gory to get the spies right.

[simonbuchan]

Added some unit tests - might be a bit ugly?

[AllanZhengYP]

The test update looks good, but I need to clean it up a bit. I will merge it and post a follow up PR!

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.