Skip to content

Use WinHttpCloseHandle from another thread to cancel the http request. #568

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.

Sign up for GitHub

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

Closed
smibe wants to merge 3 commits into from
Closed

Use WinHttpCloseHandle from another thread to cancel the http request. #568

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aca6ff2
Use WinHttpCloseHandle from another thread to cancel the http request.
Gerhard-Herbert Jun 30, 2021
047b2c6
Fixed formatting.
Gerhard-Herbert Jul 1, 2021
4841459
Cancel eventually hanging request only after trying to shut down the …
Gerhard-Herbert Jul 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions src/transports/sentry_transport_winhttp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ typedef struct {
sentry_rate_limiter_t *ratelimiter;
HINTERNET session;
HINTERNET connect;
HINTERNET request;
bool debug;
} winhttp_bgworker_state_t;

Expand Down Expand Up @@ -110,6 +111,28 @@ 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 (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);

@Swatinem Swatinem Jul 1, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation why the ordering is important. It might be a good idea to put that as a comment in the code.

I also saw that all the tests are failing, I think in that case also the ordering is important.
The main thread will ask the background thread to shut down here according to the timeout. I think you have to move all your close calls after this call here, because you do want to wait for the given timeout, instead of just throwing away all the requests that are currently in flight.

}

Expand All @@ -129,7 +152,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];
Expand All @@ -152,10 +174,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;
Expand All @@ -178,16 +200,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,
(DWORD)req->body_len, (DWORD)req->body_len, 0)) {
WinHttpReceiveResponse(request, NULL);
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) {
// 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);

Expand All @@ -197,7 +219,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)) {
Expand All @@ -211,15 +233,15 @@ 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);
if (h) {
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);
Expand All @@ -238,7 +260,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);
Expand Down