fix: Initialize curl in the correct order #341
Conversation
This also checks more thoroughly for both curl and winhttp state.
Codecov Report
@@ Coverage Diff @@
## master #341 +/- ##
==========================================
- Coverage 87.59% 87.37% -0.23%
==========================================
Files 49 49
Lines 3998 4039 +41
==========================================
+ Hits 3502 3529 +27
- Misses 496 510 +14 |
| (void)scope; | ||
| } | ||
| if (backend && backend->user_consent_changed_func) { | ||
| backend->user_consent_changed_func(backend); | ||
| } | ||
|
|
||
| // after initializing the transport, we will submit all the unsent envelopes | ||
| // and handle remaining sessions. | ||
| sentry__process_old_runs(options); |
There was a problem hiding this comment.
The old order of execution was: options -> transport -> old runs -> backend.
The new order of execution is: transport -> backend -> options -> old runs.
Are we sure that it is fine to clean up old runs after initializing the backend?
There was a problem hiding this comment.
The transport previously required the global options but that was fixed already. The problem with the backend is that it currently uses "public" API internally, which requires the global options.
The other option would be to set the options, and afterwards unset them again in case the backends fails to init, which would be observable from the outside, which is also weird.
The cleanest thing to do would be to explicitly pass the options around, but that is hard to do right now.
All-in-all, I think options should only be set once when the SDK was successfully initialized and is ready to be used.
| (void)scope; | ||
| } | ||
| if (backend && backend->user_consent_changed_func) { | ||
| backend->user_consent_changed_func(backend); |
There was a problem hiding this comment.
Shouldn't we do this in the backend then? It seems like it is the responsibility of the backend to make sure that the initial configuration is applied.
| * step in the event pipeline. | ||
| * | ||
| * Envelopes will be submitted to the transport in a _fire and forget_ fashion, | ||
| * and the transport must send those envelopes _in order_. |
There was a problem hiding this comment.
Envelopes have to be sent in order now? So the transport can't send envelopes in parallel then.
This should be noted in the CHANGELOG as a breaking change as well.
There was a problem hiding this comment.
this has been the case ever since release health, since that relies on getting the session updates in order. But sure, I can add it to the changelog as well.
This also checks more thoroughly for both curl and winhttp state.
Also reworks the transport startup to be fallible, and re-orders the startup path a little bit.