From aca6ff23908b98e088f830f91f0db723b3ce4c71 Mon Sep 17 00:00:00 2001 From: Gerhard Herbert Date: Wed, 30 Jun 2021 18:59:04 +0200 Subject: [PATCH 1/4] Use WinHttpCloseHandle from another thread to cancel the http request. --- src/transports/sentry_transport_winhttp.c | 37 +++++++++++++++++------ 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/transports/sentry_transport_winhttp.c b/src/transports/sentry_transport_winhttp.c index 516d0ec16..b7c725578 100644 --- a/src/transports/sentry_transport_winhttp.c +++ b/src/transports/sentry_transport_winhttp.c @@ -20,6 +20,7 @@ typedef struct { sentry_rate_limiter_t *ratelimiter; HINTERNET session; HINTERNET connect; + HINTERNET request; bool debug; } winhttp_bgworker_state_t; @@ -110,6 +111,21 @@ static int sentry__winhttp_transport_shutdown(uint64_t timeout, void *transport_state) { sentry_bgworker_t *bgworker = (sentry_bgworker_t *)transport_state; + winhttp_bgworker_state_t *state = sentry__bgworker_get_state(bgworker); + + if (state->connect) { + WinHttpCloseHandle(state->connect); + state->connect = NULL; + } + if (state->session) { + WinHttpCloseHandle(state->session); + state->session = NULL; + } + if (state->request) { + WinHttpCloseHandle(state->request); + state->request = 0; + } + return sentry__bgworker_shutdown(bgworker, timeout); } @@ -129,7 +145,6 @@ sentry__winhttp_send_task(void *_envelope, void *_state) wchar_t *url = sentry__string_to_wstr(req->url); wchar_t *headers = NULL; - HINTERNET request = NULL; URL_COMPONENTS url_components; wchar_t hostname[128]; @@ -152,10 +167,10 @@ sentry__winhttp_send_task(void *_envelope, void *_state) } bool is_secure = strstr(req->url, "https") == req->url; - request = WinHttpOpenRequest(state->connect, L"POST", + state->request = WinHttpOpenRequest(state->connect, L"POST", url_components.lpszUrlPath, NULL, WINHTTP_NO_REFERER, WINHTTP_DEFAULT_ACCEPT_TYPES, is_secure ? WINHTTP_FLAG_SECURE : 0); - if (!request) { + if (!state->request) { SENTRY_WARNF( "`WinHttpOpenRequest` failed with code `%d`", GetLastError()); goto exit; @@ -178,16 +193,16 @@ sentry__winhttp_send_task(void *_envelope, void *_state) SENTRY_TRACEF( "sending request using winhttp to \"%s\":\n%S", req->url, headers); - if (WinHttpSendRequest(request, headers, (DWORD)-1, (LPVOID)req->body, + if (WinHttpSendRequest(state->request, headers, (DWORD)-1, (LPVOID)req->body, (DWORD)req->body_len, (DWORD)req->body_len, 0)) { - WinHttpReceiveResponse(request, NULL); + WinHttpReceiveResponse(state->request, NULL); if (state->debug) { // this is basically the example from: // https://docs.microsoft.com/en-us/windows/win32/api/winhttp/nf-winhttp-winhttpqueryheaders#examples DWORD dwSize = 0; LPVOID lpOutBuffer = NULL; - WinHttpQueryHeaders(request, WINHTTP_QUERY_RAW_HEADERS_CRLF, + WinHttpQueryHeaders(state->request, WINHTTP_QUERY_RAW_HEADERS_CRLF, WINHTTP_HEADER_NAME_BY_INDEX, NULL, &dwSize, WINHTTP_NO_HEADER_INDEX); @@ -197,7 +212,7 @@ sentry__winhttp_send_task(void *_envelope, void *_state) // Now, use WinHttpQueryHeaders to retrieve the header. if (lpOutBuffer - && WinHttpQueryHeaders(request, + && WinHttpQueryHeaders(state->request, WINHTTP_QUERY_RAW_HEADERS_CRLF, WINHTTP_HEADER_NAME_BY_INDEX, lpOutBuffer, &dwSize, WINHTTP_NO_HEADER_INDEX)) { @@ -211,7 +226,7 @@ sentry__winhttp_send_task(void *_envelope, void *_state) // lets just assume we won’t have headers > 2k wchar_t buf[2048]; DWORD buf_size = sizeof(buf); - if (WinHttpQueryHeaders(request, WINHTTP_QUERY_CUSTOM, + if (WinHttpQueryHeaders(state->request, WINHTTP_QUERY_CUSTOM, L"x-sentry-rate-limits", buf, &buf_size, WINHTTP_NO_HEADER_INDEX)) { char *h = sentry__string_from_wstr(buf); @@ -219,7 +234,7 @@ sentry__winhttp_send_task(void *_envelope, void *_state) sentry__rate_limiter_update_from_header(state->ratelimiter, h); sentry_free(h); } - } else if (WinHttpQueryHeaders(request, WINHTTP_QUERY_CUSTOM, + } else if (WinHttpQueryHeaders(state->request, WINHTTP_QUERY_CUSTOM, L"retry-after", buf, &buf_size, WINHTTP_NO_HEADER_INDEX)) { char *h = sentry__string_from_wstr(buf); @@ -238,7 +253,9 @@ sentry__winhttp_send_task(void *_envelope, void *_state) SENTRY_TRACEF("request handled in %llums", now - started); exit: - if (request) { + if (state->request) { + HINTERNET request = state->request; + state->request = 0; WinHttpCloseHandle(request); } sentry_free(url); From 047b2c699a9243f289b68cc8a4d4749466a96986 Mon Sep 17 00:00:00 2001 From: Gerhard Herbert Date: Thu, 1 Jul 2021 11:32:47 +0200 Subject: [PATCH 2/4] Fixed formatting. --- src/transports/sentry_transport_winhttp.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/transports/sentry_transport_winhttp.c b/src/transports/sentry_transport_winhttp.c index b7c725578..a471d2ec8 100644 --- a/src/transports/sentry_transport_winhttp.c +++ b/src/transports/sentry_transport_winhttp.c @@ -123,9 +123,9 @@ sentry__winhttp_transport_shutdown(uint64_t timeout, void *transport_state) } if (state->request) { WinHttpCloseHandle(state->request); - state->request = 0; + state->request = NULL; } - + return sentry__bgworker_shutdown(bgworker, timeout); } @@ -193,8 +193,8 @@ sentry__winhttp_send_task(void *_envelope, void *_state) SENTRY_TRACEF( "sending request using winhttp to \"%s\":\n%S", req->url, headers); - if (WinHttpSendRequest(state->request, headers, (DWORD)-1, (LPVOID)req->body, - (DWORD)req->body_len, (DWORD)req->body_len, 0)) { + if (WinHttpSendRequest(state->request, headers, (DWORD)-1, + (LPVOID)req->body, (DWORD)req->body_len, (DWORD)req->body_len, 0)) { WinHttpReceiveResponse(state->request, NULL); if (state->debug) { From 4841459427146824d69a4fa585aee907daacf58b Mon Sep 17 00:00:00 2001 From: Gerhard Herbert Date: Mon, 5 Jul 2021 10:45:19 +0200 Subject: [PATCH 3/4] Cancel eventually hanging request only after trying to shut down the bg-thread regulary. --- src/transports/sentry_transport_winhttp.c | 29 ++++++++++++++--------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/transports/sentry_transport_winhttp.c b/src/transports/sentry_transport_winhttp.c index a471d2ec8..3268e72ba 100644 --- a/src/transports/sentry_transport_winhttp.c +++ b/src/transports/sentry_transport_winhttp.c @@ -113,17 +113,24 @@ sentry__winhttp_transport_shutdown(uint64_t timeout, void *transport_state) sentry_bgworker_t *bgworker = (sentry_bgworker_t *)transport_state; winhttp_bgworker_state_t *state = sentry__bgworker_get_state(bgworker); - if (state->connect) { - WinHttpCloseHandle(state->connect); - state->connect = NULL; - } - if (state->session) { - WinHttpCloseHandle(state->session); - state->session = NULL; - } - if (state->request) { - WinHttpCloseHandle(state->request); - state->request = NULL; + if (sentry__bgworker_shutdown(bgworker, timeout) != 0) { + // Seems like some requests are taking too long/hanging + // Just close them to make sure the background thread is exiting. + if (state->connect) { + WinHttpCloseHandle(state->connect); + state->connect = NULL; + } + + // NOTE: We need to close the session before closing the request. + // This will cancel all other requests which might be queued as well. + if (state->session) { + WinHttpCloseHandle(state->session); + state->session = NULL; + } + if (state->request) { + WinHttpCloseHandle(state->request); + state->request = NULL; + } } return sentry__bgworker_shutdown(bgworker, timeout); From 93bfa53d8d011e1628bbdc8c0cbab90679e13601 Mon Sep 17 00:00:00 2001 From: Arpad Borsos Date: Tue, 6 Jul 2021 11:36:47 +0200 Subject: [PATCH 4/4] correctly dump enqueued envelopes --- src/sentry_sync.c | 1 + src/transports/sentry_transport_winhttp.c | 7 ++++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/sentry_sync.c b/src/sentry_sync.c index ade4e369e..f652f6654 100644 --- a/src/sentry_sync.c +++ b/src/sentry_sync.c @@ -325,6 +325,7 @@ sentry__bgworker_shutdown(sentry_bgworker_t *bgw, uint64_t timeout) uint64_t now = sentry__monotonic_time(); if (now > started && now - started > timeout) { + sentry__atomic_fetch_and_add(&bgw->running, -1); sentry__mutex_unlock(&bgw->task_lock); SENTRY_WARN( "background thread failed to shut down cleanly within timeout"); diff --git a/src/transports/sentry_transport_winhttp.c b/src/transports/sentry_transport_winhttp.c index 3268e72ba..afb6e7c52 100644 --- a/src/transports/sentry_transport_winhttp.c +++ b/src/transports/sentry_transport_winhttp.c @@ -113,7 +113,8 @@ sentry__winhttp_transport_shutdown(uint64_t timeout, void *transport_state) sentry_bgworker_t *bgworker = (sentry_bgworker_t *)transport_state; winhttp_bgworker_state_t *state = sentry__bgworker_get_state(bgworker); - if (sentry__bgworker_shutdown(bgworker, timeout) != 0) { + int rv = sentry__bgworker_shutdown(bgworker, timeout); + if (rv != 0) { // Seems like some requests are taking too long/hanging // Just close them to make sure the background thread is exiting. if (state->connect) { @@ -133,7 +134,7 @@ sentry__winhttp_transport_shutdown(uint64_t timeout, void *transport_state) } } - return sentry__bgworker_shutdown(bgworker, timeout); + return rv; } static void @@ -262,7 +263,7 @@ sentry__winhttp_send_task(void *_envelope, void *_state) exit: if (state->request) { HINTERNET request = state->request; - state->request = 0; + state->request = NULL; WinHttpCloseHandle(request); } sentry_free(url);