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

Bug fix: Direct method subscription not removed after disconnect #3359

Conversation

tmahmood-microsoft
Copy link
Contributor

@tmahmood-microsoft tmahmood-microsoft commented Aug 7, 2023

After a disconnect event, when the client reconnects, client subscribes to message receive event again resulting in multiple calls to direct method callback.
The root cause was missing unsubscription after a disconnect event.

github issue: #3354

@tmahmood-microsoft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@abhipsaMisra abhipsaMisra left a comment

Choose a reason for hiding this comment

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

RemoveOldOperations is called at regular intervals to remove older pending twin operation tasks from our internal collection.
Unhooking the disconnection and received message event handlers in the same function isn't what you want to do, since what will happen here is, even without any disconnection event the handlers will get unhooked after a certain interval.
Instead, you need to unhook the message processor as a part of the transport handler's disconnection detection block.

@tmahmood-microsoft
Copy link
Contributor Author

Updated and unhooking the message processor as a part of the transport handler's disconnection detection block.

@abhipsaMisra
Copy link
Member

What scenarios did you test for this?
Can we add some tests so that we don't regress in the future?
For a unit test you could set up a mock client and trigger the disconnection event handler. Then, verify that the message processor handler was unhooked. As a bonus, you can also test that no other operation was executed on the client, i.e. no other handlers were attached or detached.

Copy link
Contributor

@patilsnr patilsnr left a comment

Choose a reason for hiding this comment

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

Agree about adding a test, but otherwise makes sense.

…andler is only being triggered after disconnection
@tmahmood-microsoft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmahmood-microsoft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmahmood-microsoft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmahmood-microsoft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmahmood-microsoft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmahmood-microsoft tmahmood-microsoft merged commit cfcc10b into previews/v2 Aug 9, 2023
@tmahmood-microsoft tmahmood-microsoft deleted the tmahmood/multiple-direct-method-calls-after-reconnect branch August 9, 2023 21:40
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.

4 participants