SentryHttpFailedRequestHandler disposes Content on .net framework

[Liklainy]

Package

Sentry

.NET Flavor

.NET Framework

.NET Version

4.7.2

OS

Any (not platform specific)

SDK Version

3.33.0

Self-Hosted Sentry Version

No response

Steps to Reproduce

1. Generate client from OpenApi with NSwag
2. Create HttpClient with SentryHttpMessageHandler and pass it to generated client
3. Send request which will fail and will be caught with SentryHttpFailedRequestHandler

Expected Result

No exception thrown

Actual Result

1. Handler uses EnsureSuccessStatusCode which disposes Content on older versions (before net core)
2. NSwag generated code checks status code and if it's not 200 it tries to read content body which is already disposed and generates ObjectDisposed exception

[jamescrosswell]

Hi @Liklainy, thanks for getting in touch.

@bitsandfoxes we could fairly easily implement our own EnsureSuccessStatusCode for target frameworks below .net5.0. I know Matt was against this on principal, as it would mean we wouldn't get any fixes if the code we copied changed.

However, we've already got a workaround for the broken framework code, but at the moment that workaround only protects the proper functioning of our own code (but doesn't guarantee other stuff won't get broken).

In this particular case, I'd be OK to bend the "don't copy framework code into our solution" rule, as long as our code is only used on .netcore3.1 and below. Under the hood, all EnsureSuccessStatusCode does is throw an exception if the response status code isn't in the 200 range, so trivial to implement ourselves:

• https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System/net/System/Net/Http/HttpResponseMessage.cs#L117-L120
• https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System/net/System/Net/Http/HttpResponseMessage.cs#L142-L159

[Liklainy]

Hi @jamescrosswell, thank you for quick reply!

I can suggest two ways to fix this issue:

1. As a simple and crude fix we can store Content stream locally, make it null so it will not be disposed, and restore it in finally
2. Throwing an exception just to immediately catch it doesn't make any sense. We only need to create an exception with new. But HttpRequestException has a different structure in different versions, for example, StatusCode was added as a separate property which probably should be initialized for compatibility, which leads to more #if workarounds. Maybe the best way will be to create a separate custom exception class

[bitsandfoxes]

we could fairly easily implement our own EnsureSuccessStatusCode for target frameworks below .net5.0.

I agree with this.