Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ServiceBus] Adding default idle timeout for amqpConnection object #7944

Merged
merged 3 commits into from
Oct 14, 2019

Conversation

nemakam
Copy link
Contributor

@nemakam nemakam commented Oct 5, 2019

Adding default idle timeout of 60 seconds for amqpConnection object.

@nemakam nemakam requested review from binzywu and vinaysurya October 5, 2019 01:53
@nemakam nemakam requested a review from jsquire as a code owner October 5, 2019 01:53
@nemakam nemakam self-assigned this Oct 5, 2019
@jsquire jsquire added the Client This issue points to a problem in the data-plane of the library. label Oct 7, 2019
@@ -128,7 +128,8 @@ public static AmqpConnectionSettings CreateAmqpConnectionSettings(uint maxFrameS
{
MaxFrameSize = maxFrameSize,
ContainerId = containerId,
HostName = hostName
HostName = hostName,
IdleTimeOut = (uint)Constants.DefaultOperationTimeout.TotalMilliseconds
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the behavior without the explicit setting? What's the driver behind this change? I'm curious on two fronts:

  • It likely changes the behavior of the library in a subtle way that current consumers may not expect and could possibly need to be a breaking change.

  • I'm curious if this is something that should be applied to Event Hubs as well, or if this is a case that is specific to Service Bus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So essentially, during a network communication issue, when packets are not sent being sent from client to service (or vice versa) this parameter dictates how long should the connection be alive. On windows, the connection gets dropped automatically after sometime. However, in Linux, the connection stays in the ESTABLISHED state (using netstat) even after several minutes.
From the customer view, it looks like MessageSender.Send() is called, but the operation just times out and throws ServiceBusTimeoutException instead of communication exception, since the connection never gets into faulted state.
With this change, we are forcing that 1 minute of idle time on the connection would force it to drop the connection and reestablish when required.

It likely changes the behavior of the library in a subtle way that current consumers may not expect and could possibly need to be a breaking change.

I wouldn't exactly consider it a breaking change and would think of it as a bugfix. It already works as expected on windows machine, and this would start working on linux as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if this is something that should be applied to Event Hubs as well, or if this is a case that is specific to Service Bus.

Yes. It should be applied to Event Hubs as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Neeraj. I appreciate the context.

@nemakam nemakam merged commit 31a8ecc into Azure:master Oct 14, 2019
@nemakam nemakam deleted the amqpconnectiontimeout branch October 14, 2019 22:59
jsquire added a commit to jsquire/azure-sdk-for-net that referenced this pull request Oct 17, 2019
AMQP Foundation
  - Adding an idle timeout to the AMQP connection to ensure that it does
    not hang in an inconsistent state on some platforms.
    (see: Azure#7944)

  - Restructuring of retry loops for service operations for more uniform
    exception handling and logging.

  - Adjusted close for client types to mark the object as closed before
    taking any action to ensure that the instance is not usable for normal
    operations while in a closing state; this will revert should an
    exception occur to allow for attempting to close again.

  - Removing unused annotations type; the members used have been folded into
    the AMQP Properties class, where they are a more logical fit.

  - General light tweaking and tuning AMQP-related constructs for efficiency
    and to normalize patterns.
jsquire added a commit that referenced this pull request Oct 17, 2019
AMQP Foundation
  - Adding an idle timeout to the AMQP connection to ensure that it does
    not hang in an inconsistent state on some platforms.
    (see: #7944)

  - Restructuring of retry loops for service operations for more uniform
    exception handling and logging.

  - Adjusted close for client types to mark the object as closed before
    taking any action to ensure that the instance is not usable for normal
    operations while in a closing state; this will revert should an
    exception occur to allow for attempting to close again.

  - Removing unused annotations type; the members used have been folded into
    the AMQP Properties class, where they are a more logical fit.

  - General light tweaking and tuning AMQP-related constructs for efficiency
    and to normalize patterns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Service Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants