NodeHttp2Handler does not handle session failures

[isker]

Describe the bug
NodeHttp2Handler pools its Http2Sessions per authority. An Http2Session can be closed for a variety of reasons, including those outside of the control of the client. (See the docs for the goaway event which can be triggered by the server at any time.)

Whenever a session is closed, it is invalid for further use, but NodeHttp2Handler only invalidates its cache for sessions it has closed itself. This is insufficient - if the server triggers node to close the session for any reason, the NodeHttp2Handler is invalid for that authority forever.

SDK version number
@aws-sdk/node-http-handler@1.0.0-gamma.6

Is the issue in the browser/Node.js/ReactNative?
node

Details of the browser/Node.js/ReactNative version

λ node -v
v13.9.0

To Reproduce (observed behavior)
The repro steps and context for this issue are the same as in #1524.

Expected behavior
NodeHttp2Handler should invalidate its cache on the close event for any Http2Session. Compare to another popular client:
https://github.com/hisco/http2-client/blob/12a9e6fa6701a46e92e9b7adf395ce646b2a26b4/lib/request.js#L299-L303

[TysonAndre]

I believe that a patch to fix this would look something along these lines? Note that I haven't tested or even run this - anything involving http/2 would probably require multiple unit tests - node's http/2 implementation has some edge cases in older node versions that can lead to crashes (possibly related to closing a session while it's in the process of closing)

--- a/packages/node-http-handler/src/node-http2-handler.ts
+++ b/packages/node-http-handler/src/node-http2-handler.ts
@@ -107,14 +107,31 @@ export class NodeHttp2Handler implements HttpHandler {
 
     const newSession = connect(authority);
     connectionPool.set(authority, newSession);
+    const destroySessionCb = () => {
+      this.destroySession(authority, newSession);
+    };
+    newSession.on('close' , destroySessionCb); // probably redundant?
+    newSession.on('goaway' , destroySessionCb);
+    newSession.on('error' , destroySessionCb);
+    newSession.on('frameError' , destroySessionCb);
 
     const sessionTimeout = this.sessionTimeout;
     if (sessionTimeout) {
-      newSession.setTimeout(sessionTimeout, () => {
-        newSession.close();
-        connectionPool.delete(authority);
-      });
+      newSession.setTimeout(sessionTimeout, destroySessionCb);
     }
     return newSession;
   }
+
+  /**
+   * Destroy a session and remove it from the http2 pool.
+   *
+   * This check ensures that the session is only closed once
+   * and that an event on one session does not close a different session.
+   */
+  private destroySession(authority: string, session: ClientHttp2Session): void {
+    if (this.connectionPool.get(authority) === session) {
+      this.connectionPool.delete(authority);
+      session.close();
+    }
+  }
 }

I'm moderately familiar with node's http/2 implementation - I'd proposed a fix for a similar bug in https://github.com/parse-community/node-apn/ in parse-community/node-apn#27 - the maintainers went with a different approach inspired by that code, an HTTP/2 client for APNs(Apple Push Notification Service), to tolerate spurious connection errors to HTTP/2

I'm not sure which approach you planned to take, but feel free to use this patch or base code off of the unit tests I wrote for https://github.com/parse-community/node-apn/pull/27/files#diff-ecc2f3b14c967f02fd074ac2d0e0b876e683c84f2755d251bbaa74a813a82130 (e.g. createAndStartMockServer, createAndStartMockLowLevelServer) - but I assume aws's sdk already has something similar

I'd also been involved in setting up tests for https://github.com/parse-community/node-apn/blob/master/test/client.js

• I mention this because the tests of a mock server emitting goaway on the client may be relevant

---

Also, what's the expected impact?

I'd been assuming that a spurious aws server error or networking error may lead to prompt errors (and the setTimeout not being reached), meaning that a failed connection would result in all subsequent requests to aws for that authority failing, which has made me hesitate to start using NodeHttp2Handler for a long lived node process?

---

NodeHttp2Handler should invalidate its cache on the close event for any Http2Session

I think it should be done earlier, when the first error is encountered - close is only called after the session is finished being destroyed, which would only happen after all requests on that http2 connection had completed

https://nodejs.org/api/http2.html#http2_event_close

The 'close' event is emitted once the Http2Session has been destroyed. Its listener does not expect any arguments.

[TysonAndre]

https://github.com/hisco/http2-client/blob/12a9e6fa6701a46e92e9b7adf395ce646b2a26b4/lib/request.js#L306-L320

The linked client also checks the .closed property before closing to see if it was already closed, and removes all event listeners of the client

[hari-007]

Is this below error from client-lex-runtime-v2 also relates to this?.

It is happening after the bot session timeout time, even though I am on the putSession call !!

• If so, when are you (@trivikr or @TysonAndre) planning to push this high priority labeled patch?

  • Also, I saw that fix [fix(node-http-handler): handle NodeHttp2Handler session failure fix(node-http-handler): handle NodeHttp2Handler session failure #2289] got merged 4 days back and wondering what would be the release/publish a policy for this kind of important issues

• If not, I want to open an issue for this as it is forcing me to restart the node server

Any simple reply would be appreciated.

[TysonAndre]

Is this below error from client-lex-runtime-v2 also relates to this?.

That seems likely to be related to this - the intent of the PR was to fix issues like that when http/2 was used (reconnect for destroyed sessions) so that I could safely use clients such as dynamo (and various others) with http/2

The PR you linked wasn't merged yet and will likely be fixed by that PR - 3.14.0 was published on April 30th and the PR was merged 5 days later on May 5th.

If so, when are you (@trivikr or @TysonAndre) planning to push this high priority labeled patch?

I'm an external contributor to AWS, not a maintainer, so I can't publish anything, but releases seem to typically be on fridays?

Unit tests were added to confirm that disconnections initiated by the server
resulted in the connection being removed from the connection pool.

More thorough manual tests would be useful to test all possible networking edge cases,
e.g. completely losing a connection to aws load balancers, a connection closing without a goaway frame, etc,
testing this with a high volume of traffic to see if it recovers properly, etc.,
but I'm not familiar with how to simulate that accurately in node

I'd want to have it properly tested however the aws team normally tests changes rather than rushing to merge it out

[trivikr]

Closing as the fix was released in v3.15.0.

Please reopen or comment if the issue still exists.

[hari-007]

@trivikr : I am still able to reproduce this issue. And it is not happening with V2 though.

• It is exactly happening after 5 minutes of idle action. Also, so consistent (very exact 5 minute window, it will throw the session timeout)
• Everything is the same as I commented above. Not working with 3.14.0, 3.15.0, and 3.16.0 too
• For now, I am switching back to V2 library as my project timeline closing. Hope someone can reproduce, fix, and release soon. @TysonAndre : Got you, thanks for the quick reply.

Demo git project to reproduce this:

• Here is the demo reproducable git repo for > node-aws-lex-bot-communicator-demo
  • I created this demo project to show case this session failure scenario with Lex client.
  • I have given all the instructions for how to reproduce in that repo ReadMe.MD file
  • Along with the session destroyed, I often got the Connection reset error like below :

[TysonAndre]

It is exactly happening after 5 minutes of idle action. Also, so consistent (very exact 5 minute window, it will throw the session timeout)

https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-target-groups.html#target-group-attributes

I'm not sure if this is actually the issue (I'm not familiar with client-lex-runtime-v2 and don't work for aws/amazon), but I suspect it may be related to the load balancer idle timeout.

deregistration_delay.timeout_seconds
The amount of time for Elastic Load Balancing to wait before deregistering a target. The range is 0–3600 seconds. The default value is 300 seconds. (5 minutes)

[TysonAndre]

@hari-007 in case it's an issue that only happens on older node versions, what node major&minor version (node --version) are you using in production?

[TysonAndre]

+  private destroySession(authority: string, session: ClientHttp2Session): void {
+    if (this.connectionPool.get(authority) !== session) {
+      // Already closed?
+      return;
+    }
+    this.connectionPool.delete(authority);
+    session.removeAllListeners("goaway");
+    session.removeAllListeners("error");
+    session.removeAllListeners("frameError");

This seems like it may have been overly broad - the implementation was based on another one mentioned earlier in the link - https://github.com/hisco/http2-client/blob/12a9e6fa6701a46e92e9b7adf395ce646b2a26b4/lib/request.js#L306-L320
(in case the lack of listener cleanup would lead to a leak - I'm not sure if that's necessary)

I was assuming the http2 client wouldn't register its own listeners, but I'm thinking that assumption is a mistake - https://nodejs.org/api/events.html - if it was registering its own listeners before/after aws-sdk registered one or during shutdown to suppress thrown exceptions, then removeAllListeners would cause issues