From 8ed11ab98bb013bc0e3554b6728d30e9a2f9a431 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 17 Apr 2024 21:02:32 +1200 Subject: [PATCH 1/4] Fixed: SentryHttpFailedRequestHandler disposes Content on .net framework --- .../Extensions/HttpStatusExtensions.cs | 28 +++++++++++++ src/Sentry/SentryHttpFailedRequestHandler.cs | 30 +++++++------- .../HttpRequestExceptionMessageTests.cs | 41 +++++++++++++++++++ 3 files changed, 85 insertions(+), 14 deletions(-) create mode 100644 src/Sentry/Internal/Extensions/HttpStatusExtensions.cs create mode 100644 test/Sentry.Tests/Internals/Extensions/HttpRequestExceptionMessageTests.cs diff --git a/src/Sentry/Internal/Extensions/HttpStatusExtensions.cs b/src/Sentry/Internal/Extensions/HttpStatusExtensions.cs new file mode 100644 index 0000000000..e7f8aa41d6 --- /dev/null +++ b/src/Sentry/Internal/Extensions/HttpStatusExtensions.cs @@ -0,0 +1,28 @@ +namespace Sentry.Internal.Extensions; + +#if !NET5_0_OR_GREATER +internal static class HttpStatusExtensions +{ + private const string HttpRequestExceptionMessage = "Response status code does not indicate success: {0}"; + + /// + /// This mimics the behaviour of for netcore3.0 and later + /// by throwing an exception if the status code is outside the 200 range without disposing the content. + /// + /// See https://github.com/getsentry/sentry-dotnet/issues/2684 + /// + /// + /// + public static void EnsureSuccessStatusCode(this HttpStatusCode statusCode) + { + if ((int)statusCode < 200 || (int)statusCode > 299) + { + throw new HttpRequestException(string.Format( + CultureInfo.InvariantCulture, + HttpRequestExceptionMessage, + (int)statusCode + )); + } + } +} +#endif diff --git a/src/Sentry/SentryHttpFailedRequestHandler.cs b/src/Sentry/SentryHttpFailedRequestHandler.cs index 008125ede8..05caef6b7a 100644 --- a/src/Sentry/SentryHttpFailedRequestHandler.cs +++ b/src/Sentry/SentryHttpFailedRequestHandler.cs @@ -1,3 +1,4 @@ +using Sentry.Internal.Extensions; using Sentry.Protocol; namespace Sentry; @@ -19,23 +20,17 @@ protected internal override void DoEnsureSuccessfulResponse([NotNull] HttpReques return; } -#if NET5_0_OR_GREATER - // Starting with .NET 5, the content and headers are guaranteed to not be null. - var bodySize = response.Content.Headers.ContentLength; -#else - // We have to get the content body size before calling EnsureSuccessStatusCode, - // because older implementations of EnsureSuccessStatusCode disposes the content. - // See https://github.com/dotnet/runtime/issues/24845 - - // The ContentLength might be null (but that's ok). - // See https://github.com/dotnet/runtime/issues/16162 - var bodySize = response.Content?.Headers?.ContentLength; -#endif - // Capture the event try { +#if NET5_0_OR_GREATER response.EnsureSuccessStatusCode(); +#else + // Use our own implementation of EnsureSuccessStatusCode because older implementations of + // EnsureSuccessStatusCode disposes the content. + // See https://github.com/dotnet/runtime/issues/24845 + response.StatusCode.EnsureSuccessStatusCode(); +#endif } catch (HttpRequestException exception) { @@ -54,7 +49,14 @@ protected internal override void DoEnsureSuccessfulResponse([NotNull] HttpReques var responseContext = new Response { StatusCode = (short)response.StatusCode, - BodySize = bodySize +#if NET5_0_OR_GREATER + // Starting with .NET 5, the content and headers are guaranteed to not be null. + BodySize = response.Content.Headers.ContentLength +#else + // The ContentLength might be null (but that's ok). + // See https://github.com/dotnet/runtime/issues/16162 + BodySize = response.Content?.Headers?.ContentLength +#endif }; if (!Options.SendDefaultPii) diff --git a/test/Sentry.Tests/Internals/Extensions/HttpRequestExceptionMessageTests.cs b/test/Sentry.Tests/Internals/Extensions/HttpRequestExceptionMessageTests.cs new file mode 100644 index 0000000000..2f53df1ba2 --- /dev/null +++ b/test/Sentry.Tests/Internals/Extensions/HttpRequestExceptionMessageTests.cs @@ -0,0 +1,41 @@ +namespace Sentry.Tests.Internals.Extensions; + +#if !NET5_0_OR_GREATER +public class HttpRequestExceptionMessageTests +{ + [Fact] + public void EnsureSuccessStatusCode_StatusCodeInRange_DoesNotThrow() + { + // Arrange + const HttpStatusCode statusCode = HttpStatusCode.OK; // Any status code in the 200-299 range + + // Act + var act = () => statusCode.EnsureSuccessStatusCode(); + + // Assert + act.Should().NotThrow(); + } + + [Fact] + public void EnsureSuccessStatusCode_StatusCodeOutOfRange_ThrowsHttpRequestException() + { + var unsuccessfulStatusCodes = new List(); + unsuccessfulStatusCodes.AddRange(Enumerable.Range(0, 199)); + unsuccessfulStatusCodes.AddRange(Enumerable.Range(300, 600)); + foreach (var i in unsuccessfulStatusCodes) + { + // Arrange and Act + var statusCode = (HttpStatusCode)i; + var act = () => statusCode.EnsureSuccessStatusCode(); + + // Assert + act.Should().Throw() + .WithMessage(string.Format( + CultureInfo.InvariantCulture, + "Response status code does not indicate success: {0}", + (int)statusCode + )); + } + } +} +#endif From e990d9d42bb1f425b58d82ecc0910165e24bc9d3 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 17 Apr 2024 21:04:59 +1200 Subject: [PATCH 2/4] Update CHANGELOG.md --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2144faaa7..315c474bc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Fixed: SentryHttpFailedRequestHandler disposes Content on .net framework ([#3306](https://github.com/getsentry/sentry-dotnet/pull/3306)) + ## 4.4.0 ### Features From 4ecff9f7c1e178221f855e0abd51e2d55f9d80e8 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Wed, 17 Apr 2024 22:38:46 +1200 Subject: [PATCH 3/4] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 315c474bc7..1029f3c415 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixes -- Fixed: SentryHttpFailedRequestHandler disposes Content on .net framework ([#3306](https://github.com/getsentry/sentry-dotnet/pull/3306)) +- response.Content is not disposed by SentryHttpFailedRequestHandler on .net framework ([#3306](https://github.com/getsentry/sentry-dotnet/pull/3306)) ## 4.4.0 From 2c29e6333dab5d0b14ac21d036f4f1e8fd4d6698 Mon Sep 17 00:00:00 2001 From: James Crosswell Date: Thu, 18 Apr 2024 20:13:23 +1200 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1029f3c415..e98879e12b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixes -- response.Content is not disposed by SentryHttpFailedRequestHandler on .net framework ([#3306](https://github.com/getsentry/sentry-dotnet/pull/3306)) +- `HttpResponse.Content` is no longer disposed by when using `SentryHttpFailedRequestHandler` on .NET Framework, which was causing an ObjectDisposedException when using Sentry with NSwag ([#3306](https://github.com/getsentry/sentry-dotnet/pull/3306)) ## 4.4.0