(fix): Build error due to missing dispose #1768
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vinagesh
commented
Feb 2, 2021
vinagesh
commented
Feb 2, 2021
vinagesh
requested review from
abhipsaMisra,
azabbasi,
barustum,
bikamani,
drwill-ms,
jamdavi and
timtay-microsoft
as code owners
drwill-ms
reviewed
Feb 2, 2021
iothub/device/src/DeviceAuthenticationWithRegistrySymmetricKey.cs
Outdated
Show resolved
Hide resolved
drwill-ms
reviewed
Feb 2, 2021
drwill-ms
approved these changes
Feb 2, 2021
abhipsaMisra
reviewed
Feb 2, 2021
|
We'll also need to update AuthenticationWithTokenRefresh to have it not implement IDisposable again |
vinagesh
force-pushed
the
vinagesh/dispose
branch
from
5f82936
to
7324a53
Compare
Will remove |
vinagesh
force-pushed
the
vinagesh/dispose
branch
from
7324a53
to
0ce38d5
Compare
vinagesh
commented
Feb 3, 2021
abhipsaMisra
reviewed
Feb 3, 2021
vinagesh
force-pushed
the
vinagesh/dispose
branch
2 times, most recently
from
0ce38d5
to
40e4a45
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
abhipsaMisra
reviewed
Feb 3, 2021
bikamani
reviewed
Feb 3, 2021
iothub/device/src/ModuleAuthenticationWithRegistrySymmetricKey.cs
Outdated
Show resolved
Hide resolved
bikamani
approved these changes
Feb 3, 2021
vinagesh
force-pushed
the
vinagesh/dispose
branch
2 times, most recently
from
d852b38
to
f2979e1
Compare
vinagesh
force-pushed
the
vinagesh/dispose
branch
4 times, most recently
from
df5e871
to
cd17f05
Compare
vinagesh
force-pushed
the
vinagesh/dispose
branch
from
cd17f05
to
5386a8f
Compare
|
Tested on the main pipeline and everything works well now. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
drwill-ms
approved these changes
Feb 4, 2021
vinagesh
commented
Feb 4, 2021
| Certificate?.Dispose(); | ||
| Certificate = null; | ||
| #endif | ||
| IotHubConnectionString?.TokenRefresher?.Dispose(); |
There was a problem hiding this comment.
I decided to dispose whatever is needed here. Making the IotHubConnectionString class did not seem right as we really do not want that dispose to be called anywhere else.
timstewartm
pushed a commit
to timstewartm/azure-iot-sdk-csharp
that referenced
this pull request
May 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
masterbranch.Description of the changes
Some implementations of IAuthenticationMethod have objects that need to be disposed. And we were not disposing because the interface did not have IDisposable and the implementations did not dispose anything.
This change makes the interface disposable and hence any implementation that has disposable objects, dispose them. It is a no-op for the rest.
This also means, the public IotHubConnectionStringBuilder is now IDisposable. This could be a breaking change but something necessary.