Repository navigation
Deadlock when closing the http_listener #13
Description
Activity
I suspect this area of code is of interest:
pplx::task<void> http_windows_server::respond(http::http_response response) { windows_request_context * p_context = static_cast<windows_request_context *>(response._get_server_context()); return pplx::create_task(p_context->m_response_completed).then([p_context](::pplx::task<void> t) { // After response is sent, break circular reference between http_response and the request context. // Otherwise http_listener::close() can hang. p_context->m_response._get_impl()->_set_server_context(nullptr); t.get(); }); }It looks like if there's some exception that prevents this bit of code from running, then the circular reference isn't broken, causing the hang.
From
windows_request_context::async_process_request:const unsigned long error_code = HttpReceiveHttpRequest( pServer->m_hRequestQueue, m_request_id, 0, m_request, headers_size, NULL, &m_overlapped); if(error_code != NO_ERROR && error_code != ERROR_IO_PENDING) { CancelThreadpoolIo(pServer->m_threadpool_io); m_msg.reply(status_codes::InternalError); }I've managed to reproduce the issue by putting a breakpoint at
HttpReceiveHttpRequestinwindows_request_context::async_process_requestand waiting for the client to time out, causing a network disconnection.HttpReceiveHttpRequestreturnsERROR_CONNECTION_INVALID, which preventsm_msg.reply(status_codes::InternalError);from succeeding. The response now waits indefinitely for a broken network connection, and holds on to the smart pointer for the server context, keeping the number of outstanding requests above zero indefinitely. I wouldn't be surprised if there were other error conditions that prevented the reference from being released.I forgot to mention that I was running version 2.6 of the library.
It seems a lot of the error handling code in the
windows_request_contextclass relies onm_msg.reply(status_codes::InternalError);doing the right thing. However, if the lambda inwindows_request_context::dispatch_request_to_listenerthat runs as a response to thehttp_request.content_ready()task is never run, then bad things will happen. The callback for thehttp_request.get_response()task is not initialised, preventing replies from working, or at least be able to release the cyclic reference towindows_request_context. Also, them_msghandle won't be reset, which means there could still be a cyclic reference between thehttp_requestandwindows_request_contextif no attempt is made to send a reply.First off, thanks a bunch for this very detailed analysis. I'm going to look into reproducing the issue on my local machine.
Please continue posting any further details you find out.
One piece of information that might be useful in your analysis: The only way that the lambda in
pplx::task<void> http_windows_server::respond(http::http_response response) { windows_request_context * p_context = static_cast<windows_request_context *>(response._get_server_context()); return pplx::create_task(p_context->m_response_completed).then([p_context](::pplx::task<void> t) { // After response is sent, break circular reference between http_response and the request context. // Otherwise http_listener::close() can hang. p_context->m_response._get_impl()->_set_server_context(nullptr); t.get(); }); }could possibly not run is if
m_response_completedis never set. Even if it is set with an exception, the continuation will still run (and the server context will be reset correctly).Thanks for the update, but that isn't entirely accurate. There are several ways in which that lambda can't run correctly. First off, is that when
HttpReceiveHttpRequestfails inwindows_request_context::async_process_request,m_response_completedis never set because no exception is thrown. Secondly, I've also noticed that if theget_response()task I mentioned fromdispatch_request_to_listenerdoesn't run (there are many ways this can fail), thenm_responseisn't initialised, so the task inhttp_windows_server::respondcan't break the cyclic reference as it's not pointing to the correct member (it's underp_context->m_msg.get_response().get()instead).I've seen an instance where the destructor of
m_response_completedtriggered the lambda, but it crashed because p_context had already been deleted. I haven't been able to reliably reproduce that situation, I've got more analysis to do.I've also found a case where
m_response_completedis set with an exception, but since no response was created (and thus the protective task-based continuation fromdetails::_http_request::_reply_implisn't registered), the destructor ofm_response_completedtakes down the whole process with an unobserved exception violation. This occurs whenHttpReceiveRequestEntityBodyreturns a serious (non-EOF) failure insidewindows_request_context::read_request_body_chunk.In case you're having difficulty reproducing, I've got my client side set to a 1 second timeout, and I'm just strategically putting in breakpoints in the cpprest library server-side, waiting for the client to time out the request, and then resuming the server.
Key breakpoint places that cause issues so far include:
- Immediately before
HttpReceiveHttpRequestinwindows_request_context::async_process_request(preventsm_response_completedbeing triggered) - Immedately before
HttpReceiveRequestEntityBodyinwindows_request_context::read_request_body_chunk(causesm_response_completedunobserved exception)
Additionally, forcing the client to send a malformed URL causes a similar failure. The function
dispatch_request_to_listeneris not called, preventing registration of theget_response()lambda and therefore preventingm_response_completedfrom being signalled.- Immediately before
I'm currently in the process of drafting a patch for a pull request to hopefully resolve some/all of the issues I've identified, however there are some questions I'd like answered before continuing.
- There are quite a few places that call
m_msg.reply(...)with some kind of error code. The replies aren't being returned because the callbacks required to process the reply are conditionally initialised insidewindows_request_context::dispatch_request_to_listener. There's a guard with a comment saying "If an exception occurred while processing the body then there is no reason to even try sending the response, just re-surface the same exception." This goes against the whole point in sending the error replies back to the client. Is there some justification for blocking the reply? I don't want to reintroduce an old bug if there's a specific reason for this. - The code ensures that the response can't be sent until the request body has been fully received. The current state of the code has lost the comment that explains that this is intentional, but I didn't find a specific reason why this should be the case. If there's an error in the response header, or other internal error, I'm not sure why we'd need to wait for the request body before informing the client of the error reply. What issue does this delay resolve?
- The "asio" based listener breaks out the reply callbacks from the
connection::dispatch_request_to_listenerfunction, so that error replies are properly handled. Is there a specific reason that the "httpsys" variant doesn't follow this model? It's the model I'm planning on basing my patch on. While comparing the two implementations, I did notice that the "asio" version doesn't handle thetask_canceledexception in theget_responsehandler, I'd suggest that this functionality from the "httpsys" version should be ported over.
- There are quite a few places that call
- I can think of no reason why we have to block the response. Not blocking it shouldn't be an issue by itself, though it might exacerbate any other bugs that are lurking (example: connection was dropped during transmission, so sending a response is going to fail. Due to this being a relatively rare scenario, the code may have other bugs). I don't think that's a reason to stop your change though; the objective is to fix the bugs, not cover them up :).
- I don't believe the delay is required, similarly to the above. The original intent was probably to conceptually simplify the implementation since it's easier to think about "Ok, now we receive the request. Ok, that's done. Ok, now we send the response." than to consider all the possible mixed-states that can arise when simultaneously processing the request and response. For example, there might be data races around
m_msgwhere the request continuation resets it while the response continuation is reading from it. - Yeah, that sounds like a good idea. I guess as it currently works, the asio implementation will always respond with at least an
InternalError? I'm always in favor of reducing the differences between the platforms, so bringing them in line here would be great.
Additionally, I want to again say thanks for putting more eyes on this code. The
http_listenercode is the least tested code in the library despite its age, so there are certainly a lot of issues here that need fixing.I've pushed another change for review. I'm currently stress testing it as we speak.
I found an answer as to why the library currently delays the response until the body download is complete. The
m_overlappedmember is reused between the body fetch and response send functions, so we don't want to race over that member. We also want to make sure that, since them_responsemember now owns thestd::unique_ptrof the context, we need to maintain ownership of the response so it doesn't go out of scope mid way through the content still fetching.I'm pretty sure there's still a race condition lurking, where if
m_response_completedis signalled with an error before them_responsemember is initialised, then the callback that breaks the circular reference will run too early. I'd like to move the creation of that callback implementation to the point wherem_responseis initialised, however I'm not sure what the implications of that will be on other observers of the task returned byhttp_windows_server::respond.Can I get my pull request reviewed please? My boss is asking what the hold up is. Unless any issues are raised during the review, I think the pull request should be ready to merge.
Alright, the pull request has been merged. Thanks for the ping reminder :)
Duplicated from CodePlex (issue 421) since the documentation says that issues should be reported to GitHub instead:
When calling http_listener.close().wait() as we're shutting down our service, the application will occasionally deadlock because the wait operation never returns. The deadlock appears to be in the http_windows_server::stop() function, there's a wait that never returns.