feat(native): add public custom HTTP transport client interface - #1987
feat(native): add public custom HTTP transport client interface#1987HuzaifaAbdulRehman wants to merge 7 commits into
Conversation
jpnurmi
left a comment
There was a problem hiding this comment.
Nice, thanks! Awesome start, looking good so far. I left a few suggestions and discussion points.
d40c0a9 to
a8a9717
Compare
jpnurmi
left a comment
There was a problem hiding this comment.
Very nice, LGTM 👍
I'll give it a try with Qt just to see how it feels and that we didn't miss anything
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1987 +/- ##
==========================================
+ Coverage 74.31% 74.37% +0.05%
==========================================
Files 104 104
Lines 25638 25745 +107
Branches 4627 4650 +23
==========================================
+ Hits 19054 19147 +93
- Misses 5281 5291 +10
- Partials 1303 1307 +4 🚀 New features to boost your workflow:
|
a8a9717 to
6be491e
Compare
The only thing that stands out is that our static int qt_http_client_send(sentry_http_client_t *client, sentry_http_request_t *request, sentry_http_response_t *response)
{
QNetworkAccessManager *nam = static_cast<QtHttpClient *>(client)->nam;
QNetworkReply *reply = nam->sendCustomRequest(...);
QEventLoop loop;
QObject::connect(reply, &QNetworkReply::finished, &loop, &QEventLoop::quit);
loop.exec();
auto status = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute);
sentry_http_response_set_status_code(response, status.toInt());
...
}However, I'm hesitant to complicate things and split the flow into separate
calls because it would create a whole range of new issues... 🤔 |
|
Yeah I ran into the same thought. I'd still keep it synchronous though. The retry, ordering and caching all depend on knowing how a request went before deciding what's next, and splitting it up would mean rebuilding all that around pending state. curl and winhttp are both synchronous too, so it fits the rest of the code. The Qt side might be less scary than it looks though, since send_func runs on the background worker thread, not the main one. So that nested QEventLoop is spinning on a thread with no UI and no user code, which avoids most of the usual re-entrancy headaches. One gotcha I noticed while checking: the factory and the start hook both run on the calling thread, but send_func runs on the worker thread. So a QNetworkAccessManager created in either of those would end up with the wrong thread affinity. Creating it lazily on the first send_func call should sort it. Happy to document that thread split if you think it's worth calling out, feels like an easy one to trip over. |
|
I think the current flow is ok. Let's ignore the Qt-related finding - not a real issue. The only remaining issue is the dubious shutdown flag, which I'm frankly not quite sure what to do with... |
|
Happy to just drop it from the public API if you're unsure. Easier to add it back later than take it away, and it means clients don't have to think about it at all. I had a look at what the flag actually does and it's not much. It only really changes things for envelopes with attachment refs, and even then the envelope still gets persisted either way, just through a different path. So if the transport just assumed any failure after shutdown starts is a shutdown failure, I don't think we'd lose anything meaningful. Would be a small change, just an atomic flag on the transport state like the bgworker already does. Want me to push that, or leave it as is? |
Sure, go for it. And thanks for willing to iterate, very much appreciated! |
Expose sentry_http_transport_new so applications can plug in their own HTTP client (platform-native ones like Qt, .NET, or Dart) while sentry-native keeps owning request queueing, envelope ordering, retry with exponential backoff, offline caching, rate-limit handling, and client reports. The curl and WinHTTP transports are now implemented against this same interface, deduplicating the Sentry-specific response header parsing that used to be copy-pasted in both.
- Document that both body accessors return NULL for bodyless requests (e.g. the TUS creation POST), and cover that case in the accessor tests - Add a sentry_http_client_t typedef to distinguish client pointers from factory/user-data pointers in the public API - Rename the sentry_prepared_http_request_s tag to sentry_http_request_s so the public opaque type no longer leaks the old internal name - Document that the client shutdown hook runs on the sentry_close thread, concurrently with an in-flight send_func call on the transport thread - Fix a test that leaked its client object by allocating one instead of using a non-owning fake pointer when exercising the null-free-func path
…header The forward-declared-tag pattern is used throughout the codebase (envelope, options, transport, scope, ...), so spelling it out here adds no information.
The 0.16.3 release cut the Unreleased heading while this branch was open, so the rebase left this entry inside the released section.
Custom HTTP clients had to call `sentry_http_response_set_shutdown` to say that a request failed only because the transport was shutting down. That put the burden on every client implementation, and required them to be thread safe even in the current single-instance case, since the shutdown hook runs on the `sentry_close` thread. The transport already knows when it is shutting down, so it can classify the failure itself. Adds an atomic `shutting_down` flag set at the start of `http_transport_shutdown`, before the client is ever asked to stop, and reads it in `http_send_request` instead of the client-reported value. Removes `sentry_http_response_set_shutdown` from the public API.
6be491e to
f7258e0
Compare
|
Pushed it. sentry_http_response_set_shutdown is gone from the public API, the transport now sets an atomic flag at the start of http_transport_shutdown and treats any send_func failure after that as a shutdown failure. Set before the client is ever asked to stop, so there's no window where a shutdown-caused failure gets read as a normal error. Also rebased on master to pick up the changelog fix. |
On Windows `sentry_path_t` keeps the path both as canonical UTF-8 (`path`) and as wide chars (`path_w`), and the SDK's rule is to use the wide Win32 APIs when leaving the SDK boundary, since the narrow ones interpret `char *` according to the ANSI code page rather than UTF-8. `sentry_http_request_get_body_file_path` only exposed the narrow variant, so a custom client passing it to `fopen`/`CreateFileA` would fail to open any path containing non-ASCII characters. Both built-in transports avoid this by reading `path_w` directly (`CreateFileW` in WinHTTP, `_wfopen` in curl), which a custom client had no way to do. Adds `sentry_http_request_get_body_file_pathw`, following the same `w` suffix convention already used by `sentry_options_set_database_pathw` and friends, and documents the encoding on the narrow accessor.
jpnurmi
left a comment
There was a problem hiding this comment.
This looks great, thank you so much for contributing a significant feature!
This will become very handy for downstream SDKs (e.g. getsentry/sentry-dart#3551) wishing to replace the curl dependency without losing all the functionality provided by the built-in HTTP transport. 🎉
@JoshuaMoelans would appreciate 👀 if you have time 🙏
|
Thanks! Glad it'll be useful for the Dart side, I hadn't seen that issue before. Happy to address anything Joshua flags. |
Closes #1979.
Problem
The only public custom transport API today is
sentry_transport_new, which takes a raw envelope-send function. Using it means giving up everything the built-in HTTP transport already provides: retry with exponential backoff, offline caching, rate-limit handling, client report attachment, and the background worker/flush/shutdown machinery. Downstream SDKs that want to use a platform-native HTTP client (Qt, .NET, Dart, ...) currently have to reimplement all of that themselves.Solution
Adds
sentry_http_transport_new, a public equivalent of the internalsentry__http_transport_newclient interface. A custom HTTP client only has to execute a request and report back the status code and headers; sentry-native keeps owning request preparation/serialization, queueing, envelope ordering, retry/backoff, offline caching, rate-limit handling, client reports, and flush/shutdown.New public surface:
sentry_http_request_t/sentry_http_response_t, opaque handles, following the same forward-declared-in-sentry.hpattern already used forsentry_transport_t.get_method,get_url,get_header_count/get_header,get_body,get_body_file_path(the last one covers TUS large-attachment streaming).set_status_code,set_header.sentry_http_client_factory_func_t+sentry_http_transport_new(factory, factory_data, send_func, client_free_func).sentry_http_transport_set_client_start_func/set_client_shutdown_func.All marked
SENTRY_EXPERIMENTAL_API.Following up on the design discussion in the issue
sentry_http_response_set_headermatches the signature you proposed. curl and WinHTTP no longer parseretry-after/x-sentry-rate-limits/locationindependently, they both funnel through this one function now, so they're effectively reference implementations of the public interface rather than a separate code path.sentry_http_client_factory_func_tis called once today, but nothing in its signature assumes that. A future multi-threaded transport could call it once per worker thread without a breaking change.A note on client ownership
Initially I had
client_free_funcas a separate post-construction setter (mirroring the existingstart/shutdownhooks). That leaves a leak window: iffactory()succeeds but transport construction then fails, there's no free hook registered yet to clean up the client. Fixed by foldingclient_free_funcinto thesentry_http_transport_newconstructor itself, matching how curl.c/winhttp.c already co-locate client creation with their free function.Testing
scripts/check-clang-format.py, clean.Checklist
make style)