feat: replace libcurl with .NET HttpClient for sentry-native#4222
feat: replace libcurl with .NET HttpClient for sentry-native#4222
Conversation
jpnurmi
force-pushed
the
feat/native-transport
branch
3 times, most recently
from
a5dfd32 to
5b8a794
Compare
jpnurmi
changed the title
jpnurmi
force-pushed
the
feat/native-transport
branch
from
711e47f to
8764d14
Compare
jpnurmi
changed the title
|
jpnurmi
force-pushed
the
feat/native-transport
branch
from
f81841c to
25efbb2
Compare
jpnurmi
requested review from
Flash0ver,
aritchie and
jamescrosswell
as code owners
| _logger = logger; | ||
| } | ||
|
|
||
| protected override Task SerializeToStreamAsync(Stream stream, TransportContext? context) |
There was a problem hiding this comment.
question: overload
I believe there is also a virtual overload of SerializeToStreamAsync that takes a CancellationToken cancellationToken.
Would we like to override this overload as well, so that we also support asynchronous cancellation?
(And then have this method call into the added method passing CancellationToken.None)
There was a problem hiding this comment.
Supposedly, it needs some NET5_0_OR_GREATER guards to retain compatibility with older targets. Do you want to do that in this PR, or is that something that could be done in SerializableHttpContent as a separate step?
There was a problem hiding this comment.
Oh ... right ... our SerializableHttpContent, that means this also applies to EnvelopeHttpContent.
Good call!
Yes. Let's improve that in a follow-up in a separate PR, because EnvelopeHttpContent currently does not support asynchronous cancellation either, for NET5_0_OR_GREATER.
| #if NET5_0_OR_GREATER | ||
| var response = client.Send(request); | ||
| #else | ||
| var response = client.SendAsync(request).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
@Flash0ver might want to add a comment here that we only go through this native transport in crashes so we dorm care about blocking and that there's no risk of deadlock since no synchronization context
There was a problem hiding this comment.
Sorry if I was too eager to merge this. I wanted to get rid of the libcurl installation steps for #4247. :)
3 participants
Install a custom function transport for sentry-native, and send native envelopes with the .NET HttpClient instead of relying on the default libcurl-based HTTP transport in sentry-native. This way, sentry-native can be built with
-DSENTRY_TRANSPORT=noneto eliminate the libcurl dependency, which is problematic for minimal/chiseled .NET Native AOT containers.The behavior remains virtually identical compared to the current native transport with libcurl. The Native AOT integration test that captures a native envelope on restart passes without any changes. The only difference is that native envelopes are sent with .NET HttpClient instead of libcurl. There's no native -> .NET envelope conversion nor hooks being called.
Close: #4116