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

Unsubscribe from connection loss events when closing service client AMQP connection #3237

Merged
merged 5 commits into from
Apr 6, 2023

Conversation

timtay-microsoft
Copy link
Member

@timtay-microsoft timtay-microsoft commented Apr 3, 2023

There is no reason to notify users that the connection was lost when they are actively trying to close the connection.

I also made the other callbacks in these AMQP using clients work asynchronously just like #3224 did for the file upload notification processor callbacks.

…MQP connection

There is no reason to notify users that the connection was lost when they are actively trying to close the connection.
Copy link
Contributor

@drwill-ms drwill-ms left a comment

Choose a reason for hiding this comment

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

You'll need to update the E2E test that I added to ensure the error processor gets called. I'd suggest changing it to ensuring it does NOT get called after a call to CloseAsync, and consider adding another test to ensure it does get called, maybe if the user code throws in the FileUploadNotificationProcessor callback?

@timtay-microsoft
Copy link
Member Author

timtay-microsoft commented Apr 5, 2023

You'll need to update the E2E test that I added to ensure the error processor gets called. I'd suggest changing it to ensuring it does NOT get called after a call to CloseAsync, and consider adding another test to ensure it does get called, maybe if the user code throws in the FileUploadNotificationProcessor callback?

Sure, I'll do that, too. I'm also working on a proper "reconnection" sample for these AMQP service clients.

Edit: I'll save the sample for another PR to keep this one focused.

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft timtay-microsoft merged commit dd658b0 into previews/v2 Apr 6, 2023
@timtay-microsoft timtay-microsoft deleted the timtay/unsub branch April 6, 2023 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants