[Service Bus] Bug fix: batching receiver upon a disconnect #13374

HarshaNalluru · 2021-01-25T17:14:38Z

Bug

receiveMessages method returned zero messages upon a connection refresh caused by a network disconnect(simulated in the test).
OperationTimeout error on message settlement after the disconnect

These two failures made the "disconnect" test occasionally fail for the batching receiver.

Cause

onDetached on the batchReceivers is called 300ms after the connection refresh causing the recovered receive link to be closed.

If the message returned from the service took close to 300ms until reached the receiver since the refresh, onDetached is called to close the link leading to the loss of messages.
If the 300ms had elapsed right before the settlement, we'd see the OperationTimeout error on settlement since the receive link is closed.

Investigated here #13339

Fix

Call onDetached for the batching receivers before calling the refresh connection
And retain calling onDetached for the streaming receivers after the refresh connection

Changes in the PR

Refactored "calling onDetached" part
Removed the 300ms delay since we don't see the utility
Changelog
TODO: What to do for sessions?
- Needs more investigation [Service Bus] Bug fix: batching receiver upon a disconnect #13374 (comment), will be handled at [Service Bus] Port batching receiver improvements to sessions #8875

HarshaNalluru · 2021-01-25T17:16:07Z

sdk/servicebus/service-bus/src/connectionContext.ts

      if (!state.wasConnectionCloseCalled && state.numReceivers) {
        logger.verbose(
          `[${connectionContext.connection.id}] connection.close() was not called from the sdk and there were ${state.numReceivers} ` +
            `receivers. We should reconnect.`
        );
-        await delay(Constants.connectionReconnectDelay);


Removed the delay for all kinds of receivers.

HarshaNalluru · 2021-01-25T17:21:50Z

sdk/servicebus/service-bus/src/connectionContext.ts

+        await callOnDetachedOnReceivers(
+          connectionContext,
+          connectionError || contextError,
+          "streaming"


Calling onDetached on streaming receivers after the refreshConnection since onDetached would recover the streaming receiver and that would only be possible after the connection is refreshed.

Walking through this mentally it seems like it should be fine. I believe the sequence we'll end up with is something like this:

'connection refreshing promise started'

onDetach called on all receivers

batchingReceiver.onDetach

batchingReceiver.closeLink

batchingReceiver.openLock is now locked

connection refresh initiates

connection closed

new connection created

refresh connection promise resolved

(continues) batchingReceiver.closeLink

batchingReceiver.openLock is now acquired

batchingReceiver.closeLink completes (against old connection, but that's fine

batchingReceiver.openLink happens (against new connection, which is what you intend)

Does that match your analysis?

One correction..

The closeLink ends completely and then the refresh is initiated.

Thanks for sharing this reasoning! Any chance you can put it as a comment as well for our future selves?

Added more comments. :)

sdk/servicebus/service-bus/src/connectionContext.ts

richardpark-msft · 2021-01-25T18:30:53Z

sdk/servicebus/service-bus/src/core/batchingReceiver.ts

@@ -93,7 +93,7 @@ export class BatchingReceiver extends MessageReceiver {
      );
    }

-    await this._batchingReceiverLite.close(connectionError);
+    this._batchingReceiverLite.close(connectionError);


Looking at this now it seems 'close' is a bit misnamed - it's more like "cancel" or something of that nature. (ie, it doesn't change the link state at all).

(not something you need to fix in this PR, just curious if you agree)

Hmmm... maybe terminate? (since it refers to ending the things than canceling or closing)

Logged #13390

richardpark-msft

This looks good to me!

sdk/servicebus/service-bus/src/connectionContext.ts

Verify the receiverType that's passed

sdk/servicebus/service-bus/src/connectionContext.ts

chradek · 2021-01-25T23:34:10Z

sdk/servicebus/service-bus/src/connectionContext.ts

+
+  for (const receiverName of Object.keys(connectionContext.messageReceivers)) {
+    const receiver = connectionContext.messageReceivers[receiverName];
+    if (receiver && receiver.receiverType === receiverType) {


Does this affect session receivers at all? Just want to make sure we're still calling onDetached on all receivers like we used to now that we're checking receiverType as well.

onDetached was(and is) only called for the streaming and batching receivers.

(Session receivers can only be accessed through connectionContext.messageSessions. We never called onDetached for sessions, there is no onDetached method that is implemented for sessions.)

Previously, onDetached was called for "batching" and "streaming" receivers after the refresh connection.
Now, onDetached is called for "batching" before the refresh connection, unlike the streaming receivers.

I left a TODO to investigate further on the "sessions" part, I'll log an issue for it too.

There is this related issue #8875 which intends to deal with the work related to disconnects for sessions, that issue should take care of the remaining work for sessions, not logging a new issue.

Added a comment with questions - #8875 (comment)

Co-authored-by: chradek <[email protected]>

…ivers after the refresh connection

…b.com/HarshaNalluru/azure-sdk-for-js into harshan/sb/issue/13339-disconnect-fix

HarshaNalluru · 2021-01-26T00:33:03Z

/azp run js - servicebus - tests

azure-pipelines · 2021-01-26T00:33:13Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2021-01-26T01:54:24Z

Hello @HarshaNalluru!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

HarshaNalluru added 2 commits January 25, 2021 09:11

New helper method - callOnDetachedOnReceivers

f450d8d

unneeded await

19bf713

ghost added the Service Bus label Jan 25, 2021

fix jsdoc

f3cece0

HarshaNalluru commented Jan 25, 2021

View reviewed changes

no ondetached for sessions?

af78050

HarshaNalluru commented Jan 25, 2021

View reviewed changes

sdk/servicebus/service-bus/src/connectionContext.ts Outdated Show resolved Hide resolved

HarshaNalluru commented Jan 25, 2021

View reviewed changes

sdk/servicebus/service-bus/src/connectionContext.ts Outdated Show resolved Hide resolved

richardpark-msft reviewed Jan 25, 2021

View reviewed changes

richardpark-msft approved these changes Jan 25, 2021

View reviewed changes

getNumberOfReceivers helper method and fix numbers

4775ed0

HarshaNalluru commented Jan 25, 2021

View reviewed changes

sdk/servicebus/service-bus/src/connectionContext.ts Show resolved Hide resolved

HarshaNalluru added 3 commits January 25, 2021 12:40

receiver.receiverType === receiverType

85eda12

Verify the receiverType that's passed

receiver.receiverType

7e78332

changelog

a1135af

HarshaNalluru marked this pull request as ready for review January 25, 2021 20:54

HarshaNalluru requested review from chradek and ramya-rao-a as code owners January 25, 2021 20:54

chradek reviewed Jan 25, 2021

View reviewed changes

sdk/servicebus/service-bus/src/connectionContext.ts Outdated Show resolved Hide resolved

chradek reviewed Jan 25, 2021

View reviewed changes

HarshaNalluru and others added 3 commits January 25, 2021 15:55

Update sdk/servicebus/service-bus/src/connectionContext.ts

e9d199b

Co-authored-by: chradek <[email protected]>

Comments explaining why we call the onDetached for the streaming rece…

8f5f4d6

…ivers after the refresh connection

Merge branch 'harshan/sb/issue/13339-disconnect-fix' of https://githu…

677fb45

…b.com/HarshaNalluru/azure-sdk-for-js into harshan/sb/issue/13339-disconnect-fix

This was referenced Jan 26, 2021

[DEBUG] Flaky test - Batch receiver disconnect - 100 iterations #13339

Closed

[Service Bus] Port batching receiver improvements to sessions #8875

Closed

[Service Bus] (Minor) Internal rename #13390

Closed

HarshaNalluru requested a review from chradek January 26, 2021 01:04

HarshaNalluru requested a review from richardpark-msft January 26, 2021 01:04

chradek approved these changes Jan 26, 2021

View reviewed changes

HarshaNalluru added the auto-merge label Jan 26, 2021

ghost merged commit 6cda731 into Azure:master Jan 26, 2021

richardpark-msft mentioned this pull request Jan 28, 2021

[Service Bus] In flight messages get ignored in batch receiver if timer runs out #6270

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Service Bus] Bug fix: batching receiver upon a disconnect #13374

[Service Bus] Bug fix: batching receiver upon a disconnect #13374

HarshaNalluru commented Jan 25, 2021 •

edited

Loading

HarshaNalluru Jan 25, 2021 •

edited

Loading

HarshaNalluru Jan 25, 2021 •

edited

Loading

richardpark-msft Jan 25, 2021 •

edited

Loading

HarshaNalluru Jan 25, 2021

chradek Jan 25, 2021

HarshaNalluru Jan 26, 2021

richardpark-msft Jan 25, 2021

richardpark-msft Jan 25, 2021

HarshaNalluru Jan 25, 2021

HarshaNalluru Jan 26, 2021

richardpark-msft left a comment

chradek Jan 25, 2021

HarshaNalluru Jan 26, 2021 •

edited

Loading

HarshaNalluru Jan 26, 2021 •

edited

Loading

HarshaNalluru commented Jan 26, 2021

azure-pipelines bot commented Jan 26, 2021

ghost commented Jan 26, 2021

[Service Bus] Bug fix: batching receiver upon a disconnect #13374

[Service Bus] Bug fix: batching receiver upon a disconnect #13374

Conversation

HarshaNalluru commented Jan 25, 2021 • edited Loading

Bug

Cause

Fix

Changes in the PR

HarshaNalluru Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

HarshaNalluru Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

richardpark-msft Jan 25, 2021 • edited Loading

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

Choose a reason for hiding this comment

HarshaNalluru Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

HarshaNalluru Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

HarshaNalluru commented Jan 26, 2021

azure-pipelines bot commented Jan 26, 2021

ghost commented Jan 26, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

HarshaNalluru commented Jan 25, 2021 •

edited

Loading

HarshaNalluru Jan 25, 2021 •

edited

Loading

HarshaNalluru Jan 25, 2021 •

edited

Loading

richardpark-msft Jan 25, 2021 •

edited

Loading

HarshaNalluru Jan 26, 2021 •

edited

Loading

HarshaNalluru Jan 26, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.