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

Add Interceptor Overload for Send #694

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Add Interceptor Overload for Send #694

merged 2 commits into from
Dec 11, 2023

Conversation

hwoodiwiss
Copy link
Member

@hwoodiwiss hwoodiwiss commented Dec 9, 2023

This PR adds an override to InterceptingHttpMessageHandler for the synchronous Send method of DelegatingHandler to allow for calls to HttpClient.Send to be intercepted.

First pass includes some pretty icky sync-over-async 😢 , however, to avoid this would require some significant re-writes for what feels like a bit of an edge-case.

I've added this, as after upgrading a project to .NET 8 with AWSSDK.DynamoDBv2 versions > 3.7.300, I had some failing tests due to some AWS calls not being intercepted due to using the synchronous HttpClient.Send overload (here)

@hwoodiwiss hwoodiwiss requested a review from a team as a code owner December 9, 2023 19:46
@hwoodiwiss hwoodiwiss marked this pull request as draft December 9, 2023 19:46
Copy link

codecov bot commented Dec 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bf7c9a9) 96.92% compared to head (681ef94) 96.93%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #694   +/-   ##
=======================================
  Coverage   96.92%   96.93%           
=======================================
  Files          15       15           
  Lines         846      847    +1     
  Branches      193      193           
=======================================
+ Hits          820      821    +1     
  Misses         22       22           
  Partials        4        4           
Flag Coverage Δ
linux 96.93% <100.00%> (+<0.01%) ⬆️
macos 96.93% <100.00%> (+<0.01%) ⬆️
windows 96.93% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hwoodiwiss hwoodiwiss marked this pull request as ready for review December 9, 2023 20:00
@martincostello
Copy link
Member

martincostello commented Dec 9, 2023

I've added this, as after upgrading a project to .NET 8 with AWSSDK.DynamoDBv2 versions > 3.7.300, I had some failing tests due to some AWS calls not being intercepted due to using the synchronous HttpClient.Send overload

Ahhhhhhh. So that explains why that wasn't working. I'd noticed the interception wasn't working in one of our internal libraries, but I'd been able to work around it in the short term by disabling it from fetching the table metadata like noted in this blog post: https://aws.amazon.com/blogs/developer/improved-dynamodb-initialization-patterns-for-the-aws-sdk-for-net/

Mystery solved.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

Let's try out the version from the PR with the internal library on Monday to check it solves the stuff I'd run into as well. If it does then we can do a 4.1(?) or whatever the next minor version will be.

@@ -0,0 +1 @@
#nullable enable
Copy link
Member

Choose a reason for hiding this comment

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

The new bit should go in the unshipped files until it actually gets to NuGet, then it'll get moved post-release.

However that's probably not worth doing if we just ship this on Monday with no other changes.

#if NET6_0_OR_GREATER
/// <inheritdoc />
protected override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken) =>
SendAsync(request, cancellationToken).ConfigureAwait(false).GetAwaiter().GetResult();
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is a bit ick, but as it's a test library and probably quite a minority subset of cases (as it's taken 2 years to hit this) this is fine.

@martincostello martincostello added this to the v4.1.0 milestone Dec 9, 2023
@martincostello martincostello added the enhancement A change that enhances existing functionality or documentation. label Dec 9, 2023
@hwoodiwiss
Copy link
Member Author

hwoodiwiss commented Dec 9, 2023

Let's try out the version from the PR with the internal library on Monday to check it solves the stuff I'd run into as well. If it does then we can do a 4.1(?) or whatever the next minor version will be.

Sounds good to me, I inherited the interceptor with this change added in the application I ran into the sync call issue in and it seemed to work, so 🤞🏼

@martincostello
Copy link
Member

Changes validated internally.

@martincostello martincostello merged commit 701dd55 into main Dec 11, 2023
13 checks passed
@martincostello martincostello deleted the support-sync-send branch December 11, 2023 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that enhances existing functionality or documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants