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

Removed blocking call which was producing deadlocks from EC2Plugin. #234

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

samunro
Copy link
Contributor

@samunro samunro commented Mar 8, 2022

The calls to Task.Result in GetToken and GetMetadata are blocking and can result in deadlocks if either the response or the timeout are handled on the same thread. See here for a similar example and explanation.

"Don’t block on Tasks; use async all the way down."
https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

One option here could have been to replace the calls to Result with the async and await keywords, but that would have required that modifications to the IPlugin interface and its consumers. That might be something that should be considered, but given that the call to Result was already making this code synchronous, nothing will be lost by using a synchronous API as an immediate fix.

There is a synchronous Send method which has been added to HttpClient in .Net 5 but it is not available in any of the targeted frameworks - .Net Framework 4.5, .Net Core 3.1 or .Net Standard 2.0.

Although HttpWebRequest is not recommended for new development, it does offer a synchronous alternative and is the option that I have used in this pull request.

"We don't recommend that you use HttpWebRequest for new development. Instead, use the System.Net.Http.HttpClient class."
https://docs.microsoft.com/en-us/dotnet/api/system.net.httpwebrequest?view=netframework-4.5

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@samunro samunro requested a review from a team as a code owner March 8, 2022 12:34
@srprash
Copy link
Collaborator

srprash commented Mar 17, 2022

Hi @samunro
Thanks for identifying this issue and providing the fix. We should switch to using async and await in our next major version bump.

@srprash srprash merged commit 975f356 into aws:master Mar 17, 2022
@samunro
Copy link
Contributor Author

samunro commented Mar 17, 2022

Thanks @srprash. Are there any plans to release a new NuGet package? The last one was in April last year.

https://www.nuget.org/packages/AWSXRayRecorder.Core/

@srprash
Copy link
Collaborator

srprash commented Mar 17, 2022

We are working on some operation stuff for our build system and plan to do the release which has been due for some time now. Thanks for your patience.

srprash added a commit that referenced this pull request Mar 21, 2022
* Create stale.yml (#136)

* Release commit for v2.9.0 (#137)

* Added 2 seconds timeout for EC2 metadata requests (#138)

* Update NOTICE so it only has third party licenses and no proprietary notice (#139)

* Fixing link to EFCore usage section (#143)

* Added EF Core installation instruction step (#145)

* Fixing race condition for initializing and running timer in RulePoller (#154)

* Bump AWSSDK.Core dependency version (#155)

* Bump AWSSDK.Core dependency version

* Bumped AWSSDK.Core version as range.

* Removed redundant dependency

* Applied HttpClient to get sampling info (#159)

* Applied HttpWebRequest to get sampling info and removed dependency on AWSSDK.XRay

* Fixed lock issue and switched to HttpClient

* Added and modified documentations.

* Added documentations.

* Set ExpectContinue header as false by default

* Adding limitation for EFCore with multiple DbCommandInterceptor (#164)

* Adding limitation for EFCore with multiple DbCommandInterceptor

* Adding doc ref for AsyncLocal

* Updated xraycontext to use a concurrent dictionary (#157)

To avoid concurrent thread access to the dictionary causing an exception

* Enable tracing S3 id pairs and fix SNS naming issue. (#168)

* Enable tracing S3 id pairs and fix SNS naming issue.

* Changed AWSSDK.Core version

* Changed parameter names and updated comments.

* Set parameters as default

* Fixed appvoyer building configuration issue. (#173)

* Add tracing support for EntityFramework 6 (.NET Framework) (#171)

* Added support for EntityFramework 6 for .NET framework.

* Added release workflow (#179)

* Added release workflow

* Added space line

* Update configuration

* Switching out AppVeyor for GH workflow (#181)

* Switching out AppVeyor for GH workflow

* Running workflow on linux as well. building wth release config

* Revert "Switching out AppVeyor for GH workflow (#181)" (#185)

This reverts commit 34134f0.

* Added public key token (#186)

* Release commit for v2.10.0 (#187)

* Updated API Doc (#188)

* Add smoke test of distribution channel (#189)

* Add smoke test of distribution channel

* Updated AWSXRayRecorder.sln

* Points to the latest version

* Publish metric on distribution availability (#190)

* Run test on ubuntu (#191)

* Bump System.Net.Http from 4.3.3 to 4.3.4 in /sdk/src/Core (#193)

Bumps System.Net.Http from 4.3.3 to 4.3.4.

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Release commit for v2.10.1 (#194)

* Update continuous-monitoring.yml

* Increase continuous monitoring to 10 minutes

* Add netcoreapp3.1 target framework

Add a target framework moniker for .NET Core 3.1.

* Create CODEOWNERS

* Update appveyor.yml (#226)

* Update appveyor.yml

* Update appveyor.yml

* Update .netappcore version for smoke tests (#228)

* Update continuous-monitoring.yml

* Update netcoreapp version in .csproj file

* Update continuous-monitoring.yml

* Update AWSXRayRecorder.SmokeTests.csproj

* Use GitHub workflow for CI on PRs and master builds (#231)

* Update continuous-monitoring.yml

* Add Source Link support for enhanced debugging experience (#229)

* Add Source Link support for enhanced debugging experience

* NetFramework does not support embedded pdb

Co-authored-by: Jon Armen <jarmen@bstglobal.com>

* Finding the same exception in different subsegments should reuse the same cause id (#210)

Co-authored-by: Jon Armen <jarmen@bstglobal.com>

* Removed blocking call which was producing deadlocks from EC2Plugin. (#234)

* Adding 3.x for continous build

Co-authored-by: srprash <50466688+srprash@users.noreply.github.com>
Co-authored-by: Anuraag Agrawal <aanuraag@amazon.co.jp>
Co-authored-by: Lu Peng <61207760+lupengamzn@users.noreply.github.com>
Co-authored-by: Stephen D'Olier <stephen@dolier.net>
Co-authored-by: Bhautik Pipaliya <56270044+bhautikpip@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: William Armiros <54150514+willarmiros@users.noreply.github.com>
Co-authored-by: martincostello <martin@martincostello.com>
Co-authored-by: Jon Armen <jarmen@bstglobal.com>
Co-authored-by: Scott Munro <scott.munro@yahoo.com>
jon-armen added a commit to jon-armen/aws-xray-sdk-dotnet that referenced this pull request Apr 15, 2022
* Create stale.yml (aws#136)

* Release commit for v2.9.0 (aws#137)

* Added 2 seconds timeout for EC2 metadata requests (aws#138)

* Update NOTICE so it only has third party licenses and no proprietary notice (aws#139)

* Fixing link to EFCore usage section (aws#143)

* Added EF Core installation instruction step (aws#145)

* Fixing race condition for initializing and running timer in RulePoller (aws#154)

* Bump AWSSDK.Core dependency version (aws#155)

* Bump AWSSDK.Core dependency version

* Bumped AWSSDK.Core version as range.

* Removed redundant dependency

* Applied HttpClient to get sampling info (aws#159)

* Applied HttpWebRequest to get sampling info and removed dependency on AWSSDK.XRay

* Fixed lock issue and switched to HttpClient

* Added and modified documentations.

* Added documentations.

* Set ExpectContinue header as false by default

* Adding limitation for EFCore with multiple DbCommandInterceptor (aws#164)

* Adding limitation for EFCore with multiple DbCommandInterceptor

* Adding doc ref for AsyncLocal

* Updated xraycontext to use a concurrent dictionary (aws#157)

To avoid concurrent thread access to the dictionary causing an exception

* Enable tracing S3 id pairs and fix SNS naming issue. (aws#168)

* Enable tracing S3 id pairs and fix SNS naming issue.

* Changed AWSSDK.Core version

* Changed parameter names and updated comments.

* Set parameters as default

* Fixed appvoyer building configuration issue. (aws#173)

* Add tracing support for EntityFramework 6 (.NET Framework) (aws#171)

* Added support for EntityFramework 6 for .NET framework.

* Added release workflow (aws#179)

* Added release workflow

* Added space line

* Update configuration

* Switching out AppVeyor for GH workflow (aws#181)

* Switching out AppVeyor for GH workflow

* Running workflow on linux as well. building wth release config

* Revert "Switching out AppVeyor for GH workflow (aws#181)" (aws#185)

This reverts commit 34134f0.

* Added public key token (aws#186)

* Release commit for v2.10.0 (aws#187)

* Updated API Doc (aws#188)

* Add smoke test of distribution channel (aws#189)

* Add smoke test of distribution channel

* Updated AWSXRayRecorder.sln

* Points to the latest version

* Publish metric on distribution availability (aws#190)

* Run test on ubuntu (aws#191)

* Bump System.Net.Http from 4.3.3 to 4.3.4 in /sdk/src/Core (aws#193)

Bumps System.Net.Http from 4.3.3 to 4.3.4.

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Release commit for v2.10.1 (aws#194)

* Update continuous-monitoring.yml

* Increase continuous monitoring to 10 minutes

* Add netcoreapp3.1 target framework

Add a target framework moniker for .NET Core 3.1.

* Create CODEOWNERS

* Update appveyor.yml (aws#226)

* Update appveyor.yml

* Update appveyor.yml

* Update .netappcore version for smoke tests (aws#228)

* Update continuous-monitoring.yml

* Update netcoreapp version in .csproj file

* Update continuous-monitoring.yml

* Update AWSXRayRecorder.SmokeTests.csproj

* Use GitHub workflow for CI on PRs and master builds (aws#231)

* Update continuous-monitoring.yml

* Add Source Link support for enhanced debugging experience (aws#229)

* Add Source Link support for enhanced debugging experience

* NetFramework does not support embedded pdb

Co-authored-by: Jon Armen <jarmen@bstglobal.com>

* Finding the same exception in different subsegments should reuse the same cause id (aws#210)

Co-authored-by: Jon Armen <jarmen@bstglobal.com>

* Removed blocking call which was producing deadlocks from EC2Plugin. (aws#234)

* Adding 3.x for continous build

Co-authored-by: srprash <50466688+srprash@users.noreply.github.com>
Co-authored-by: Anuraag Agrawal <aanuraag@amazon.co.jp>
Co-authored-by: Lu Peng <61207760+lupengamzn@users.noreply.github.com>
Co-authored-by: Stephen D'Olier <stephen@dolier.net>
Co-authored-by: Bhautik Pipaliya <56270044+bhautikpip@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: William Armiros <54150514+willarmiros@users.noreply.github.com>
Co-authored-by: martincostello <martin@martincostello.com>
Co-authored-by: Jon Armen <jarmen@bstglobal.com>
Co-authored-by: Scott Munro <scott.munro@yahoo.com>
jon-armen added a commit to jon-armen/aws-xray-sdk-dotnet that referenced this pull request Apr 15, 2022
* Create stale.yml (aws#136)

* Release commit for v2.9.0 (aws#137)

* Added 2 seconds timeout for EC2 metadata requests (aws#138)

* Update NOTICE so it only has third party licenses and no proprietary notice (aws#139)

* Fixing link to EFCore usage section (aws#143)

* Added EF Core installation instruction step (aws#145)

* Fixing race condition for initializing and running timer in RulePoller (aws#154)

* Bump AWSSDK.Core dependency version (aws#155)

* Bump AWSSDK.Core dependency version

* Bumped AWSSDK.Core version as range.

* Removed redundant dependency

* Applied HttpClient to get sampling info (aws#159)

* Applied HttpWebRequest to get sampling info and removed dependency on AWSSDK.XRay

* Fixed lock issue and switched to HttpClient

* Added and modified documentations.

* Added documentations.

* Set ExpectContinue header as false by default

* Adding limitation for EFCore with multiple DbCommandInterceptor (aws#164)

* Adding limitation for EFCore with multiple DbCommandInterceptor

* Adding doc ref for AsyncLocal

* Updated xraycontext to use a concurrent dictionary (aws#157)

To avoid concurrent thread access to the dictionary causing an exception

* Enable tracing S3 id pairs and fix SNS naming issue. (aws#168)

* Enable tracing S3 id pairs and fix SNS naming issue.

* Changed AWSSDK.Core version

* Changed parameter names and updated comments.

* Set parameters as default

* Fixed appvoyer building configuration issue. (aws#173)

* Add tracing support for EntityFramework 6 (.NET Framework) (aws#171)

* Added support for EntityFramework 6 for .NET framework.

* Added release workflow (aws#179)

* Added release workflow

* Added space line

* Update configuration

* Switching out AppVeyor for GH workflow (aws#181)

* Switching out AppVeyor for GH workflow

* Running workflow on linux as well. building wth release config

* Revert "Switching out AppVeyor for GH workflow (aws#181)" (aws#185)

This reverts commit 34134f0.

* Added public key token (aws#186)

* Release commit for v2.10.0 (aws#187)

* Updated API Doc (aws#188)

* Add smoke test of distribution channel (aws#189)

* Add smoke test of distribution channel

* Updated AWSXRayRecorder.sln

* Points to the latest version

* Publish metric on distribution availability (aws#190)

* Run test on ubuntu (aws#191)

* Bump System.Net.Http from 4.3.3 to 4.3.4 in /sdk/src/Core (aws#193)

Bumps System.Net.Http from 4.3.3 to 4.3.4.

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Release commit for v2.10.1 (aws#194)

* Update continuous-monitoring.yml

* Increase continuous monitoring to 10 minutes

* Add netcoreapp3.1 target framework

Add a target framework moniker for .NET Core 3.1.

* Create CODEOWNERS

* Update appveyor.yml (aws#226)

* Update appveyor.yml

* Update appveyor.yml

* Update .netappcore version for smoke tests (aws#228)

* Update continuous-monitoring.yml

* Update netcoreapp version in .csproj file

* Update continuous-monitoring.yml

* Update AWSXRayRecorder.SmokeTests.csproj

* Use GitHub workflow for CI on PRs and master builds (aws#231)

* Update continuous-monitoring.yml

* Add Source Link support for enhanced debugging experience (aws#229)

* Add Source Link support for enhanced debugging experience

* NetFramework does not support embedded pdb

Co-authored-by: Jon Armen <jarmen@bstglobal.com>

* Finding the same exception in different subsegments should reuse the same cause id (aws#210)

Co-authored-by: Jon Armen <jarmen@bstglobal.com>

* Removed blocking call which was producing deadlocks from EC2Plugin. (aws#234)

* Adding 3.x for continous build

Co-authored-by: srprash <50466688+srprash@users.noreply.github.com>
Co-authored-by: Anuraag Agrawal <aanuraag@amazon.co.jp>
Co-authored-by: Lu Peng <61207760+lupengamzn@users.noreply.github.com>
Co-authored-by: Stephen D'Olier <stephen@dolier.net>
Co-authored-by: Bhautik Pipaliya <56270044+bhautikpip@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: William Armiros <54150514+willarmiros@users.noreply.github.com>
Co-authored-by: martincostello <martin@martincostello.com>
Co-authored-by: Jon Armen <jarmen@bstglobal.com>
Co-authored-by: Scott Munro <scott.munro@yahoo.com>
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.

3 participants