Use WinHttpCloseHandle from another thread to cancel the http request.#568
Use WinHttpCloseHandle from another thread to cancel the http request.#568smibe wants to merge 3 commits intogetsentry:masterfrom smibe:fix/thread-hang-on-windows
Conversation
Swatinem
left a comment
Swatinem
left a comment
There was a problem hiding this comment.
lgtm!
Since you mention a shutdown hang, will closing the handle from the main thread just lead to an error on the background thread?
| WinHttpCloseHandle(state->request); | ||
| state->request = 0; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| state->request = NULL; | ||
| } | ||
|
|
||
| return sentry__bgworker_shutdown(bgworker, timeout); |
There was a problem hiding this comment.
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.
…bg-thread regulary.
3 participants
No description provided.