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.

Sign up for GitHub

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

Jump to bottom

fix(deviceClient): Fix issue with AMQP connection pool and TokenReferesher disposal. #2260

Merged
merged 5 commits into from
Jan 7, 2022
Merged

fix(deviceClient): Fix issue with AMQP connection pool and TokenReferesher disposal. #2260

merged 5 commits into from
Jan 7, 2022

Conversation

azabbasi
Copy link
Contributor

@azabbasi azabbasi commented Dec 16, 2021 •
edited
Loading

There are two separate issues this PR is tackling.

1- When the last connection in a connection pool is disposed, we need to remove it from the connection pool map.
2- We should not dispose TokenRefereshers in connection pooling scenarios not because they are used by the AMQP connections, because it will be used in the HTTP transport handler for the file upload notification.

fixes #2235

@azabbasi
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azabbasi azabbasi marked this pull request as ready for review December 20, 2021 17:35
jamdavi
jamdavi approved these changes Dec 20, 2021
View reviewed changes
Copy link
Member

@jamdavi jamdavi left a comment

Choose a reason for hiding this comment

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

Looks good, as discussed will be looking forward to the unit test

@azabbasi
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azabbasi
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@azabbasi
Copy link
Contributor Author

azabbasi commented Jan 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azabbasi
Copy link
Contributor Author

azabbasi commented Jan 6, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azabbasi
Copy link
Contributor Author

azabbasi commented Jan 7, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drwill-ms
Copy link
Contributor

We should not dispose TokenRefereshers in connection pooling scenarios not because they are used by the AMQP connections, because it will be used in the HTTP transport handler for the file upload notification.

When will they be disposed then?

drwill-ms
drwill-ms reviewed Jan 7, 2022
View reviewed changes
iothub/device/src/IotHubConnectionString.cs
@@ -92,6 +92,33 @@ public IotHubConnectionString(IotHubConnectionStringBuilder builder)
}
}

// This constructor is only used for unit testing.
internal IotHubConnectionString(
string ioTHubName = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
string ioTHubName = null,
string iotHubName = null,

@azabbasi
Copy link
Contributor Author

azabbasi commented Jan 7, 2022

We should not dispose TokenRefereshers in connection pooling scenarios not because they are used by the AMQP connections, because it will be used in the HTTP transport handler for the file upload notification.

When will they be disposed then?

they will be disposed when the client is disposed.

@azabbasi azabbasi merged commit 8f9d565 into main Jan 7, 2022
@azabbasi azabbasi deleted the azabbasi/fixConnectionPoolingIssues branch January 7, 2022 19:22
@azabbasi azabbasi mentioned this pull request Jan 26, 2022
Merged
abhipsaMisra added a commit that referenced this pull request Feb 14, 2022
@abhipsaMisra @jamdavi
@azabbasi @drwill-ms @dylanbulfinMS
Bring pnp updates to preview (#2301)
1749f7c 
* feat(shared): Add common resources for convention-based operations

* feat(iot-device): Add support for convention-based telemetry operation

* feat(iot-device): Add support for convention-based command operations

* * feat(iot-device): Add support for convention-based properties operations

* feat(iot-device): Add support for convention-based properties operations

Co-authored-by: James Davis ⛺️🏔 <jamdavi@microsoft.com>

* feat(e2e-tests): Add telemetry E2E tests

* feat(e2e-tests): Add command E2E tests

* fix(iot-device): Updating client property collection to handle no convention

* feat(samples): Add thermostat and temperature controller sample

* fix(doc, samples): Update API design doc and move SystemTextJson helper to samples

* fix(iot-device): Separate out root-level and component-level property addition operations

* feat(tests): Add unit tests for ClientPropertyCollection

feat(tests): Add unit tests for ClientPropertyCollection

Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com>

* feat(e2e-tests): Add properties E2E tests

Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com>

* feat(e2e-tests): Add fault injection tests for properties operations (#2001)

* fix(iot-device, shared, samples): Rename StatusCodes to CommonClientResponseCodes and add a comment to highlight ClientOptions behavior

* fix(iot-device): Fix enumerator implementation to return key-value pairs

* fix(iot-device): Make ClientOptions.PayloadConvention readonly

* fix(shared): Fix from merge conflict

* fix(samples): Update the pnp samples readme (#2025)

* refactor(samples): Move preview samples to samples repository (#2057)

* fix(e2e-tests): Fix file renaming Configutaion to TestConfiguration

* refactor(iot-device): Rename property add method

* * fix(iot-device): TryGetValue methods should check for the component identifier when applicable and should not throw any exceptions (#2111)

* refactor(iot-device): Separate client reported properties into a separate accessor

* fix(iot-device): Add checks to verify that client property update response has version and requestId (#2115)

* refactor(iot-device): Update the internal method names as per functionality

* feat(iot-device): Add convenience method for acknowledging writable property update requests

* fix(iot-device): Update PayloadCollection to accept dictionary of values (#2171)

* refactor(iot-device): Merge flows for twin and client property operations (#2180)

* fix(e2e-tests): Update E2E tests to initialize and dispose resources correctly

* doc(client, service): Update documents about the Proxy property for various clients (#2248)

Update our code docs to better explain proxy settings

* Cleanup IDE warnings. From 442 down to 31. (#2254)

* Fix documented exception types thrown by methods in device client. (#2256)

* (Fix: DeviceClient): Fix the concurrency issue in MQTT stack (#2234)

* Update RegistryManager bulk API unit tests (#2258)

* feat(hub-svc): add support and tests for configurations export (#2250)

* Temporarily disable import devices/configs test (#2263)

* fix(iot-svc): update debug assert (#2264)

* fix(iot-svc): update debug assert

* Temporarily disable import devices/configs test (#2263)

* fix(iot-svc): update debug assert

* Reduce modules in ModulesClient_GetModulesOnDevice to 2

* Consolidate keyvault writes, and fix error about null (#2266)

* Cleanup code in Hub RTAC (#2268)

* fix(deviceClient): Fix issue with AMQP connection pool and TokenReferesher disposal. (#2260)

* Fix issue with AMQP connection pool

* Dispose the connection holder appropriately

* Add unit testing capability to connection pool

* Move more LA steps under conditional (#2270)

* Disable import/export of config until bug fix (#2273)

* Adding RBAC support for provisioning SDK (#2262)

* Added types for different credentials

* Altered code to work with new types

* Fixed Managers

* removed unneeded #if !NET451

* Filled in method summaries

* refactored GetHeaderProvider

* Added documentation

* Formatting fixes to address comments

* Adding Common 0.7.1 (#2272)

* Fix doc comment list bullets (#2275)

* Fix doc comment list bullets

* Change to use description tag

* sdl(all): Create SBOM for net packages  (#2261)

* Streamline an RBAC test (#2274)

* Exclude low pri .net targets in PR builds (#2277)

* fix(tests): Fix device client test to use updated API

* Update IDeviceIdentity interface to add doc comments. (#2282)

* PnP doc comment updates (#2278)

* Fix remaining doc comment bullets (#2285)

* Give e2e appId permission on the DPS instance (#2283)

* fix(DeviceClient): Avoid NRE after client dispose (#2286)

* refactor(iot-device): Update API surface for property callback API

* refactor(iot-device): Update API surface for command callback API

* fix(e2e-tests): Update E2E tests to use fluent assertions

* fix(iot-device): Update API surface to remove redundant private setters

* Revert "Adding RBAC support for provisioning SDK (#2262)" (#2289)

This reverts commit 7a26eda.

* Version bump for 2021-01-26 release (#2291)

* Update service clients instantiation doc comments (#2290)

* Update RegistryManager instantiation doc comments

* Update other service client doc comments.

* Fix class field constants casing (#2292)

* Disable checks for config in import e2e test (#2293)

* Update iteration path in vsts.yaml (#2296)

Co-authored-by: James Davis ⛺️🏔 <jamdavi@microsoft.com>
Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com>
Co-authored-by: Azad Abbasi <azabbasi@microsoft.com>
Co-authored-by: David R. Williamson <drwill@microsoft.com>
Co-authored-by: dylanbulfinMS <95251881+dylanbulfinMS@users.noreply.github.com>
timstewartm pushed a commit to timstewartm/azure-iot-sdk-csharp that referenced this pull request May 30, 2024
@azabbasi
fix(deviceClient): Fix issue with AMQP connection pool and TokenRefer…
21e3324 
…esher disposal. (Azure#2260)

* Fix issue with AMQP connection pool

* Dispose the connection holder appropriately

* Add unit testing capability to connection pool
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhipsaMisra abhipsaMisra abhipsaMisra left review comments

@jamdavi jamdavi jamdavi approved these changes

@drwill-ms drwill-ms drwill-ms approved these changes

@andyk-ms andyk-ms Awaiting requested review from andyk-ms andyk-ms is a code owner

@brycewang-microsoft brycewang-microsoft Awaiting requested review from brycewang-microsoft brycewang-microsoft is a code owner

@timtay-microsoft timtay-microsoft Awaiting requested review from timtay-microsoft timtay-microsoft is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug Report] GetFileUploadSasUriAsync throws ObjectDisposedException
4 participants
@azabbasi @drwill-ms @abhipsaMisra @jamdavi