-
-
Notifications
You must be signed in to change notification settings - Fork 200
Fix WinHTTP initialization timing to prevent error 12007 #1321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,10 @@ | |
| #include <string.h> | ||
| #include <winhttp.h> | ||
|
|
||
| #ifdef SENTRY_PLATFORM_XBOX | ||
| HRESULT sentry__transport_ensure_network_initialized(); | ||
| #endif // SENTRY_PLATFORM_XBOX | ||
|
|
||
| typedef struct { | ||
| sentry_dsn_t *dsn; | ||
| wchar_t *user_agent; | ||
|
|
@@ -40,6 +44,31 @@ sentry__winhttp_bgworker_state_new(void) | |
| return state; | ||
| } | ||
|
|
||
| static void | ||
| sentry__winhttp_session_start(void *_state) | ||
| { | ||
| winhttp_bgworker_state_t *state = _state; | ||
| if (state->proxy) { | ||
| state->session | ||
| = WinHttpOpen(state->user_agent, WINHTTP_ACCESS_TYPE_NAMED_PROXY, | ||
| state->proxy, WINHTTP_NO_PROXY_BYPASS, 0); | ||
| } else { | ||
| #if _WIN32_WINNT >= 0x0603 | ||
| state->session = WinHttpOpen(state->user_agent, | ||
| WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY, WINHTTP_NO_PROXY_NAME, | ||
| WINHTTP_NO_PROXY_BYPASS, 0); | ||
| #endif | ||
| // On windows 8.0 or lower, WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY does | ||
| // not work on error we fallback to | ||
| // WINHTTP_ACCESS_TYPE_DEFAULT_PROXY | ||
| if (!state->session) { | ||
| state->session = WinHttpOpen(state->user_agent, | ||
| WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, | ||
| WINHTTP_NO_PROXY_BYPASS, 0); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static void | ||
| sentry__winhttp_bgworker_state_free(void *_state) | ||
| { | ||
|
|
@@ -118,28 +147,14 @@ sentry__winhttp_transport_start( | |
| } | ||
| } | ||
|
|
||
| if (state->proxy) { | ||
| state->session | ||
| = WinHttpOpen(state->user_agent, WINHTTP_ACCESS_TYPE_NAMED_PROXY, | ||
| state->proxy, WINHTTP_NO_PROXY_BYPASS, 0); | ||
| } else { | ||
| #if _WIN32_WINNT >= 0x0603 | ||
| state->session = WinHttpOpen(state->user_agent, | ||
| WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY, WINHTTP_NO_PROXY_NAME, | ||
| WINHTTP_NO_PROXY_BYPASS, 0); | ||
| #endif | ||
| // On windows 8.0 or lower, WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY does | ||
| // not work on error we fallback to WINHTTP_ACCESS_TYPE_DEFAULT_PROXY | ||
| if (!state->session) { | ||
| state->session = WinHttpOpen(state->user_agent, | ||
| WINHTTP_ACCESS_TYPE_DEFAULT_PROXY, WINHTTP_NO_PROXY_NAME, | ||
| WINHTTP_NO_PROXY_BYPASS, 0); | ||
| } | ||
| } | ||
| #ifndef SENTRY_PLATFORM_XBOX | ||
| sentry__winhttp_session_start(state); | ||
|
|
||
| if (!state->session) { | ||
| SENTRY_WARN("`WinHttpOpen` failed"); | ||
| return 1; | ||
| } | ||
| #endif // !SENTRY_PLATFORM_XBOX | ||
|
|
||
| return sentry__bgworker_start(bgworker); | ||
| } | ||
|
|
@@ -187,6 +202,20 @@ sentry__winhttp_send_task(void *_envelope, void *_state) | |
| sentry_envelope_t *envelope = (sentry_envelope_t *)_envelope; | ||
| winhttp_bgworker_state_t *state = (winhttp_bgworker_state_t *)_state; | ||
|
|
||
| #ifdef SENTRY_PLATFORM_XBOX | ||
| if (!state->session) { | ||
| SENTRY_DEBUG( | ||
| "ensuring xbox network is initialized for WinHttp transport"); | ||
| sentry__transport_ensure_network_initialized(); | ||
| sentry__winhttp_session_start(state); | ||
|
Comment on lines
+209
to
+210
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to make sure - are both of these safe to call from a background thread? |
||
|
|
||
| if (!state->session) { | ||
| SENTRY_WARN("`WinHttpOpen` failed"); | ||
| return; | ||
| } | ||
| } | ||
| #endif // SENTRY_PLATFORM_XBOX | ||
|
|
||
| uint64_t started = sentry__monotonic_time(); | ||
|
|
||
| char *user_agent = sentry__string_from_wstr(state->user_agent); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,64 @@ | ||||||||||||
| #include "sentry_boot.h" | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should keep the contents here in the *winhttp.c file instead of creating a new one. So far, the mapping is file=transport. This would break that by only extending what's needed for winhttp transport.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've created dedicated file because xnetworking requires c++ compiler. Having that in mind, do you still think that it should be merged to winhttp.c?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vaind ping
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then it's probably good as is, I'll leave this up to the maintainers
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, it may be worthwile to just move this PR to sentry-xbox repository. Then this being a cpp file would be a non-issue |
||||||||||||
|
|
||||||||||||
| #include <XNetworking.h> | ||||||||||||
|
|
||||||||||||
| static void | ||||||||||||
| sentry__transport_network_connectivity_hint_changed_callback(_In_ void *context, | ||||||||||||
| _In_ const XNetworkingConnectivityHint *connectivity_hint) | ||||||||||||
| { | ||||||||||||
| HANDLE network_initialized_event = static_cast<HANDLE>(context); | ||||||||||||
| if (connectivity_hint->networkInitialized) { | ||||||||||||
| (void)SetEvent(network_initialized_event); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| extern "C" { | ||||||||||||
|
|
||||||||||||
| HRESULT | ||||||||||||
| sentry__transport_ensure_network_initialized() | ||||||||||||
| { | ||||||||||||
| HRESULT hr = S_OK; | ||||||||||||
| XNetworkingConnectivityHint connectivity_hint; | ||||||||||||
| XTaskQueueHandle queue; | ||||||||||||
|
|
||||||||||||
| hr = XTaskQueueCreate(XTaskQueueDispatchMode::Immediate, | ||||||||||||
| XTaskQueueDispatchMode::Immediate, &queue); | ||||||||||||
| if (SUCCEEDED(hr)) { | ||||||||||||
| // Use the new XNetworking APIs to check if the network is initialized. | ||||||||||||
| hr = XNetworkingGetConnectivityHint(&connectivity_hint); | ||||||||||||
| if (SUCCEEDED(hr)) { | ||||||||||||
| if (!connectivity_hint.networkInitialized) { | ||||||||||||
| // The network isn't initialized. Wait until the network becomes | ||||||||||||
| // initialized. | ||||||||||||
| HANDLE network_initialized_event | ||||||||||||
| = CreateEvent(nullptr, TRUE, FALSE, nullptr); | ||||||||||||
| if (network_initialized_event != nullptr) { | ||||||||||||
| XTaskQueueRegistrationToken token; | ||||||||||||
| hr = XNetworkingRegisterConnectivityHintChanged(queue, | ||||||||||||
| network_initialized_event, | ||||||||||||
| sentry__transport_network_connectivity_hint_changed_callback, | ||||||||||||
| &token); | ||||||||||||
| if (SUCCEEDED(hr)) { | ||||||||||||
| DWORD result = WaitForSingleObjectEx( | ||||||||||||
| network_initialized_event, INFINITE, FALSE); | ||||||||||||
|
Comment on lines
+42
to
+43
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may block indefinitely. Is that the right thing to do? I guess it's alright if we're on the background thread, but might be something to keep in mind in relation to #1316 - at the very least, add a TODO mentioning that issue
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to what we do for Switch in sentry-native/src/transports/sentry_transport_curl.c Lines 192 to 196 in d0d732e
|
||||||||||||
| if (result != WAIT_OBJECT_0) { | ||||||||||||
| hr = HRESULT_FROM_WIN32(GetLastError()); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| XNetworkingUnregisterConnectivityHintChanged( | ||||||||||||
| token, true); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| CloseHandle(network_initialized_event); | ||||||||||||
| } else { | ||||||||||||
| hr = HRESULT_FROM_WIN32(GetLastError()); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| XTaskQueueCloseHandle(queue); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return hr; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only checking session once, however, the suspend/resume cycle would influence the connectivity. Do we need to ensure connection every time we make a request instead?