Skip to content
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

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

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
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
37 changes: 27 additions & 10 deletions src/transports/sentry_transport_winhttp.c
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,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);
Swatinem marked this conversation as resolved.
Show resolved Hide resolved
state->connect = NULL;
}
if (state->session) {
WinHttpCloseHandle(state->session);
state->session = NULL;
}
if (state->request) {
WinHttpCloseHandle(state->request);
state->request = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: Shouldn’t these be closed in a particular order, in this case request first? Also, while it does not really make a difference, why is this reset to 0 while the others use NULL?

Also, since this can race with the background thread, might it be a good idea to exchange state->request first before closing it? So the racing background thread does not try to double-close it?

Copy link
Author

Choose a reason for hiding this comment

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

Yes the order is important. Closing the session first will cancel all other requests in the queue as well.
When the request handle is closed, the background blocking function is unblocked and the loop continues. But because of no session handle all WinHttp requests will fail and the queue will get cleaned up.
This is not a nice solution, but still the best I could com up with.

}

return sentry__bgworker_shutdown(bgworker, timeout);
Copy link
Member

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 +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];
Expand All @@ -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;
Expand All @@ -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);

Expand All @@ -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)) {
Expand All @@ -211,15 +226,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 +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);
Expand Down