[event-hubs] fix uncaught TypeError #8884

chradek · 2020-05-13T00:29:04Z

Summary

This change updates the EventHubReceiver, EventHubSender, and Management client to check if the rhea connection is closing before starting the process of negotiating a claim and opening a connection.

Background

More details can be found in #8584, but the short version is that the SDK sometimes ran into an edge case where it attempted to reopen a rhea connection that was in the process of being destroyed. This caused in-flight requests to throw an uncaught TypeError if the connection's state didn't update before the request timed out.

Description of changes

This change checks that connection.isOpen() === false and connection.isRemoteOpen() === true. This combination indicates that the local end of the rhea connection has closed, and rhea is giving the remote end a chance to acknowledge the close before destroying the connection.

We can see the above happen when an idle timeout is reached.

We then wait for the disconnected event to be emitted before moving on with an initialization.
Finally, we check if the connection is in the process of disconnecting and wait for it to finish.
Once all this is done, initialization can occur.

Why do we need to wait for a connection to finish disconnecting?

We don't attempt to bring back up any receivers or senders after a disconnected event, instead relying on the receivers/senders to bring themselves back up if a request failed for a retryable reason.

Even though _init waits for a disconnected event if the connection is closing, we still have to wait for the actual disconnect to complete before opening a new connection.
This is because the disconnected event listener on the ConnectionContext is non-blocking, and control will be given back to _init before the event listener completes. Since event listeners are non-blocking, putting the refresh logic in the original disconnected event listener causes a race condition because negotiateClaim can be triggered before the connection is refreshed.

chradek · 2020-05-14T05:47:04Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2020-05-14T05:47:14Z

Azure Pipelines successfully started running 1 pipeline(s).

chradek · 2020-05-14T20:54:11Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2020-05-14T20:54:21Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/eventhub/event-hubs/src/util/connectionUtils.ts

richardpark-msft · 2020-05-27T20:55:27Z

sdk/eventhub/event-hubs/src/connectionContext.ts

@@ -169,39 +198,52 @@ export namespace ConnectionContext {
      connectionContext.connection.removeAllSessions();

      // Close the cbs session to ensure all the event handlers are released.
-      await connectionContext.cbsSession.close();
+      await connectionContext.cbsSession.close().catch((err) => {


Precaution or did you actually see errors getting thrown?

I only made this change because in some of my extreme test runs I did see this throw, though it required nearly blocking the event loop for a long time.

HarshaNalluru · 2020-05-27T20:58:32Z

sdk/eventhub/event-hubs/src/connectionContext.ts


      // Close all senders and receivers to ensure clean up of timers & other resources.
      if (state.numSenders || state.numReceivers) {
        for (const senderName of Object.keys(connectionContext.senders)) {
          const sender = connectionContext.senders[senderName];
-          if (!sender.isConnecting) {


Why is this if check removed?

It's possible for disconnected to be emitted while connecting. For example, if the connection is viewed as being idle while the connection is still in the process of opening. This check was preventing us from closing the existing senders in this scenario.

I think this check is a remnant of the time when we recovered sender links when connection was disconnected. So the code below where we have sender.close() was previously sender.onDetached(). Since that need to be done if the sender is already connecting, we had this check. It should have been removed when we made the decision to not re-connect broken sender links in the background i.e. in #6737

sdk/eventhub/event-hubs/src/util/connectionUtils.ts

sdk/eventhub/event-hubs/src/managementClient.ts

richardpark-msft · 2020-05-27T21:27:50Z

sdk/eventhub/event-hubs/test/node/disconnects.spec.ts

+      // Trigger a disconnect on the underlying connection.
+      clientConnectionContext.connection["_connection"].idle();
+
+      await client.getPartitionIds({});


Might still be worth validating the results that come back, but really it's kind of a "fails completely or works" kind of call.

I added the extra verification.

richardpark-msft · 2020-05-27T21:29:30Z

sdk/eventhub/event-hubs/test/node/disconnects.spec.ts

+        // used to cause the rhea connection remote state to be cleared.
+        // This caused the in-flight sendBatch to throw an uncaught error
+        // if it timed out.
+        client.sendBatch([{ body: "test3" }]);


So this should "work"? Ie, one of our retries will pick up the new connection?

If so, can we verify that it didn't throw an exception (maybe wrap in a try/catch and assert?)

I added a check to ensure it doesn't throw. I didn't initially because I only cared that for this test, the process didn't crash, but it turned out to be trivial to add this extra assertion.

richardpark-msft

Some questions (looks like @HarshaNalluru is already asking about the isConnecting removal).

ramya-rao-a

Made a few notes after the first pass, I'll need some more time to make a second pass after the current concerns are addressed

sdk/eventhub/event-hubs/src/connectionContext.ts

ramya-rao-a · 2020-05-30T02:15:07Z

sdk/eventhub/event-hubs/src/connectionContext.ts

+    connectionContext.isConnectionClosing = function() {
+      // When the connection is not open, but the remote end is open,
+      // then the rhea connection is in the process of terminating.
+      return Boolean(!this.connection.isOpen() && this.connection.isRemoteOpen());


Is there any case when isOpen() would return true and isRemoteOpen() would return false?

I'd expect this would be possible while the connection is opening and waiting for the service to respond, but I'll confirm.
What is your concern though?

So far I've confirmed 1 case where isOpen() == true && isRemoteOpen() == false.

When opening a connection, before the service has responded that the connection is open.

I'm currently waiting to see what happens if I leave the connection open for a long time (expecting the service to initialize the close).

I also did verify that when the client calls close, isOpen() == false && isRemoteOpen() == true until the service reports that it has closed on its end.

sdk/eventhub/event-hubs/src/connectionContext.ts

sdk/eventhub/event-hubs/CHANGELOG.md

sdk/eventhub/event-hubs/src/connectionContext.ts

chradek · 2020-06-04T14:49:10Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2020-06-04T14:49:20Z

Azure Pipelines successfully started running 1 pipeline(s).

sdk/eventhub/event-hubs/src/connectionContext.ts

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

richardpark-msft · 2020-06-04T18:34:43Z

sdk/eventhub/event-hubs/src/connectionContext.ts

+/**
+ * Helper type to get the names of all the functions on an object.
+ */
+type FunctionPropertyNames<T> = { [K in keyof T]: T[K] extends Function ? K : never }[keyof T];


So...beautiful.....

I wish I could take credit but that's right out of the TypeScript handbook: https://www.typescriptlang.org/docs/handbook/advanced-types.html#distributive-conditional-types

richardpark-msft · 2020-06-04T19:00:56Z

sdk/eventhub/event-hubs/src/connectionContext.ts

+        return;
+      }
+      isDisconnecting = true;
+      waitForDisconnectPromise = new Promise((resolve) => {


It looks like the isDisconnecting variable is basically the same as waitForDisconnectPromise != null. Worth it to remove and just have the one variable to track if we're disconnecting?

Yeah...I have a check in readyToOpenLink() that checks isDisconnecting, which just felt easier to understand than if (waitForDisconnectPromise) or if (Boolean(waitForDisconnectPromise)), but it might be better to have one source of truth.

richardpark-msft

Looks good to me. Honestly, the timing on that is so tricky, good find and good fix.

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

chradek · 2020-06-04T21:43:40Z

/check-enforcer reset

[event-hubs] fix uncaught TypeError

aaaaca2

chradek requested review from ramya-rao-a and richardpark-msft as code owners May 13, 2020 00:29

chradek changed the title ~~[event-hubs] fix uncaught TypeError~~ [not-ready-for-review][event-hubs] fix uncaught TypeError May 13, 2020

chradek added 2 commits May 13, 2020 22:22

[event-hubs] create new rhea connection on disconnected

599617b

make disconnect tests node only

a76d992

chradek changed the title ~~[not-ready-for-review][event-hubs] fix uncaught TypeError~~ [event-hubs] fix uncaught TypeError May 14, 2020

some cleanup

37358ce

richardpark-msft reviewed May 27, 2020

View reviewed changes

sdk/eventhub/event-hubs/src/util/connectionUtils.ts Outdated Show resolved Hide resolved

richardpark-msft reviewed May 27, 2020

View reviewed changes

HarshaNalluru reviewed May 27, 2020

View reviewed changes

richardpark-msft reviewed May 27, 2020

View reviewed changes

sdk/eventhub/event-hubs/src/util/connectionUtils.ts Outdated Show resolved Hide resolved

richardpark-msft reviewed May 27, 2020

View reviewed changes

sdk/eventhub/event-hubs/src/managementClient.ts Outdated Show resolved Hide resolved

richardpark-msft reviewed May 27, 2020

View reviewed changes

ramya-rao-a suggested changes May 30, 2020

View reviewed changes

clean up connectionContext methods and other feedback updates

8369442

ramya-rao-a suggested changes Jun 4, 2020

View reviewed changes

chradek added 5 commits June 4, 2020 06:46

address feedback

a9baa6e

update service-bus core-amqp dep

06b8e4e

Merge remote-tracking branch 'upstream/master' into eh-fix-typeerror

1bb978a

fix test after merge

c98f6cf

fix flaky receiver test

00943d7

ramya-rao-a reviewed Jun 4, 2020

View reviewed changes

sdk/eventhub/event-hubs/src/connectionContext.ts Outdated Show resolved Hide resolved

Update sdk/eventhub/event-hubs/src/connectionContext.ts

883325d

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

richardpark-msft reviewed Jun 4, 2020

View reviewed changes

remove unneeded variable

eb0e0f2

richardpark-msft approved these changes Jun 4, 2020

View reviewed changes

ramya-rao-a approved these changes Jun 4, 2020

View reviewed changes

Update sdk/eventhub/event-hubs/CHANGELOG.md

f462956

Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>

chradek merged commit b33d7df into Azure:master Jun 4, 2020

chradek deleted the eh-fix-typeerror branch June 4, 2020 21:44

ramya-rao-a mentioned this pull request Jun 5, 2020

[Event Hubs] TypeError: Cannot read property '_process' of undefined #9311

Closed

2 tasks

chradek mentioned this pull request Jul 6, 2020

[service-bus] investigate porting TypeError fixes from event-hubs #9914

Closed

This was referenced Jul 14, 2020

[service-bus][1.x] fix uncaught TypeError #10046

Merged

[service-bus] fix uncaught TypeError #10051

Merged

ramya-rao-a mentioned this pull request Jan 31, 2021

[Service Bus] TypeError: Cannot read property '_process' of undefined #9792

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[event-hubs] fix uncaught TypeError #8884

[event-hubs] fix uncaught TypeError #8884

chradek commented May 13, 2020 •

edited

chradek commented May 14, 2020

azure-pipelines bot commented May 14, 2020

chradek commented May 14, 2020

azure-pipelines bot commented May 14, 2020

richardpark-msft May 27, 2020

chradek May 27, 2020

HarshaNalluru May 27, 2020

chradek May 27, 2020

ramya-rao-a Jun 4, 2020

richardpark-msft May 27, 2020

chradek Jun 1, 2020

richardpark-msft May 27, 2020

chradek Jun 1, 2020

richardpark-msft left a comment

ramya-rao-a left a comment

ramya-rao-a May 30, 2020

chradek Jun 1, 2020

chradek Jun 1, 2020

chradek commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

richardpark-msft Jun 4, 2020

chradek Jun 4, 2020

richardpark-msft Jun 4, 2020

chradek Jun 4, 2020

chradek Jun 4, 2020

richardpark-msft left a comment

chradek commented Jun 4, 2020

[event-hubs] fix uncaught TypeError #8884

[event-hubs] fix uncaught TypeError #8884

Conversation

chradek commented May 13, 2020 • edited

Summary

Background

Description of changes

Why do we need to wait for a connection to finish disconnecting?

chradek commented May 14, 2020

azure-pipelines bot commented May 14, 2020

chradek commented May 14, 2020

azure-pipelines bot commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

ramya-rao-a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek commented Jun 4, 2020

azure-pipelines bot commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpark-msft left a comment

Choose a reason for hiding this comment

chradek commented Jun 4, 2020

chradek commented May 13, 2020 •

edited