From 0292c7c34d12875af0774a1fe9f46190625698d2 Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Thu, 12 Nov 2015 19:45:44 +1100 Subject: [PATCH 01/10] Refactor the response handling in http_server_httpsys to fix some issues where error replies didn't get sent, and cyclic references weren't cleaned up --- .../cpprest/details/http_server_httpsys.h | 3 + .../src/http/listener/http_listener_msg.cpp | 2 +- .../src/http/listener/http_server_httpsys.cpp | 115 ++++++++++++------ 3 files changed, 79 insertions(+), 41 deletions(-) diff --git a/Release/include/cpprest/details/http_server_httpsys.h b/Release/include/cpprest/details/http_server_httpsys.h index 0de4934cf1..261aa7ad44 100644 --- a/Release/include/cpprest/details/http_server_httpsys.h +++ b/Release/include/cpprest/details/http_server_httpsys.h @@ -99,6 +99,9 @@ struct windows_request_context : http::details::_http_server_context // Dispatch request to the provided http_listener. void dispatch_request_to_listener(_In_ web::http::experimental::listener::details::http_listener_impl *pListener); + // Initialise the response task callbacks + void do_response(bool bad_request); + // Read in a portion of the request body. void read_request_body_chunk(); diff --git a/Release/src/http/listener/http_listener_msg.cpp b/Release/src/http/listener/http_listener_msg.cpp index bbfbdc99ff..15c6af57ec 100644 --- a/Release/src/http/listener/http_listener_msg.cpp +++ b/Release/src/http/listener/http_listener_msg.cpp @@ -47,7 +47,7 @@ pplx::task details::_http_request::_reply_impl(http_response response) { // Add a task-based continuation so no exceptions thrown from the task go 'unobserved'. response._set_server_context(std::move(m_server_context)); - response_completed = experimental::details::http_server_api::server_api()->respond(response); + response_completed = server_api->respond(response); response_completed.then([](pplx::task t) { try { t.wait(); } catch(...) {} diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index 21284f2c9e..f389d5fed7 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -494,6 +494,12 @@ windows_request_context::~windows_request_context() // the lock then setting of the event has completed. std::unique_lock lock(m_responseCompletedLock); + // Add a task-based continuation so no exceptions thrown from the task go 'unobserved'. + pplx::create_task(m_response_completed).then([](pplx::task t) + { + try { t.wait(); } catch(...) {} + }); + auto *pServer = static_cast(http_server_api::server_api()); if(--pServer->m_numOutstandingRequests == 0) { @@ -530,6 +536,7 @@ void windows_request_context::async_process_request(HTTP_REQUEST_ID request_id, { CancelThreadpoolIo(pServer->m_threadpool_io); m_msg.reply(status_codes::InternalError); + do_response(true); } } @@ -541,6 +548,7 @@ void windows_request_context::read_headers_io_completion(DWORD error_code, DWORD if(error_code != NO_ERROR) { m_msg.reply(status_codes::InternalError); + do_response(true); } else { @@ -574,6 +582,7 @@ void windows_request_context::read_headers_io_completion(DWORD error_code, DWORD else { m_msg.reply(status_codes::BadRequest, badRequestMsg); + do_response(true); } } } @@ -614,6 +623,7 @@ void windows_request_context::read_request_body_chunk() else { m_msg._get_impl()->_complete(0, std::make_exception_ptr(http_exception(error_code))); + m_msg._reply_if_not_already(status_codes::InternalError); } } } @@ -639,6 +649,7 @@ void windows_request_context::read_body_io_completion(DWORD error_code, DWORD by { request_body_buf.commit(0); m_msg._get_impl()->_complete(0, std::make_exception_ptr(http_exception(error_code))); + m_msg._reply_if_not_already(status_codes::InternalError); } } @@ -649,46 +660,7 @@ void windows_request_context::dispatch_request_to_listener(_In_ web::http::exper // Save http_request copy to dispatch to user's handler in case content_ready() completes before. http_request request = m_msg; - // Wait until the content download finished before replying. - request.content_ready().then([=](pplx::task requestBody) - { - // 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. - try - { - requestBody.wait(); - } - catch (...) - { - m_msg = http_request(); - cancel_request(std::current_exception()); - return; - } - - // At this point the user entirely controls the lifetime of the http_request. - m_msg = http_request(); - - request.get_response().then([this](pplx::task responseTask) - { - // Don't let an exception from sending the response bring down the server. - try - { - m_response = responseTask.get(); - } - catch (const pplx::task_canceled &) - { - // This means the user didn't respond to the request, allowing the - // http_request instance to be destroyed. There is nothing to do then - // so don't send a response. - return; - } - catch (...) - { - m_response = http::http_response(status_codes::InternalError); - } - async_process_response(); - }); - }); + do_response(false); // Look up the lock for the http_listener. auto *pServer = static_cast(http_server_api::server_api()); @@ -721,6 +693,69 @@ void windows_request_context::dispatch_request_to_listener(_In_ web::http::exper } } +void windows_request_context::do_response(bool bad_request) +{ + // Use a proxy event so we're not causing a circular reference between the http_request and the response task + pplx::task_completion_event proxy_content_ready; + + auto content_ready_task = m_msg.content_ready(); + auto get_response_task = m_msg.get_response(); + + content_ready_task.then([this, proxy_content_ready](pplx::task requestBody) + { + // 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. + try + { + requestBody.wait(); + } + catch (...) + { + m_msg = http_request(); + proxy_content_ready.set_exception(std::current_exception()); + cancel_request(std::current_exception()); + return; + } + + // At this point the user entirely controls the lifetime of the http_request. + m_msg = http_request(); + proxy_content_ready.set(); + }); + + get_response_task.then([this, bad_request, proxy_content_ready](pplx::task responseTask) + { + // Don't let an exception from sending the response bring down the server. + try + { + m_response = responseTask.get(); + } + catch (const pplx::task_canceled &) + { + // This means the user didn't respond to the request, allowing the + // http_request instance to be destroyed. There is nothing to do then + // so don't send a response. + return; + } + catch (...) + { + m_response = http::http_response(status_codes::InternalError); + } + + if (bad_request) + { + async_process_response(); + } + else + { + // Wait until the content download finished before replying. + pplx::create_task(proxy_content_ready).then([this](pplx::task requestBody) + { + async_process_response(); + }); + } + }); +} + void windows_request_context::async_process_response() { auto *pServer = static_cast(http_server_api::server_api()); From 94ab4048c44dc9760dd7c4687ccbb2aab6ba771d Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Fri, 13 Nov 2015 12:15:30 +1100 Subject: [PATCH 02/10] Fix a couple of race conditions, and use enum instead of bool --- .../cpprest/details/http_server_httpsys.h | 9 ++++- .../src/http/listener/http_server_httpsys.cpp | 40 +++++++++++-------- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/Release/include/cpprest/details/http_server_httpsys.h b/Release/include/cpprest/details/http_server_httpsys.h index 261aa7ad44..a97a461a8e 100644 --- a/Release/include/cpprest/details/http_server_httpsys.h +++ b/Release/include/cpprest/details/http_server_httpsys.h @@ -99,8 +99,13 @@ struct windows_request_context : http::details::_http_server_context // Dispatch request to the provided http_listener. void dispatch_request_to_listener(_In_ web::http::experimental::listener::details::http_listener_impl *pListener); - // Initialise the response task callbacks - void do_response(bool bad_request); + enum ShouldWaitForBody + { + WaitForBody, + DontWaitForBody + }; + // Initialise the response task callbacks. If the body has been requested, we should wait for it to avoid race conditions. + void init_response_callbacks(ShouldWaitForBody shouldWait); // Read in a portion of the request body. void read_request_body_chunk(); diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index f389d5fed7..a366641ae0 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -536,7 +536,7 @@ void windows_request_context::async_process_request(HTTP_REQUEST_ID request_id, { CancelThreadpoolIo(pServer->m_threadpool_io); m_msg.reply(status_codes::InternalError); - do_response(true); + init_response_callbacks(DontWaitForBody); } } @@ -548,7 +548,7 @@ void windows_request_context::read_headers_io_completion(DWORD error_code, DWORD if(error_code != NO_ERROR) { m_msg.reply(status_codes::InternalError); - do_response(true); + init_response_callbacks(DontWaitForBody); } else { @@ -582,7 +582,9 @@ void windows_request_context::read_headers_io_completion(DWORD error_code, DWORD else { m_msg.reply(status_codes::BadRequest, badRequestMsg); - do_response(true); + + // Even though we have a bad request, we should wait for the body otherwise we risk racing over m_overlapped + init_response_callbacks(WaitForBody); } } } @@ -623,7 +625,6 @@ void windows_request_context::read_request_body_chunk() else { m_msg._get_impl()->_complete(0, std::make_exception_ptr(http_exception(error_code))); - m_msg._reply_if_not_already(status_codes::InternalError); } } } @@ -649,7 +650,6 @@ void windows_request_context::read_body_io_completion(DWORD error_code, DWORD by { request_body_buf.commit(0); m_msg._get_impl()->_complete(0, std::make_exception_ptr(http_exception(error_code))); - m_msg._reply_if_not_already(status_codes::InternalError); } } @@ -660,7 +660,7 @@ void windows_request_context::dispatch_request_to_listener(_In_ web::http::exper // Save http_request copy to dispatch to user's handler in case content_ready() completes before. http_request request = m_msg; - do_response(false); + init_response_callbacks(WaitForBody); // Look up the lock for the http_listener. auto *pServer = static_cast(http_server_api::server_api()); @@ -693,7 +693,7 @@ void windows_request_context::dispatch_request_to_listener(_In_ web::http::exper } } -void windows_request_context::do_response(bool bad_request) +void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWait) { // Use a proxy event so we're not causing a circular reference between the http_request and the response task pplx::task_completion_event proxy_content_ready; @@ -722,7 +722,7 @@ void windows_request_context::do_response(bool bad_request) proxy_content_ready.set(); }); - get_response_task.then([this, bad_request, proxy_content_ready](pplx::task responseTask) + get_response_task.then([this, proxy_content_ready](pplx::task responseTask) { // Don't let an exception from sending the response bring down the server. try @@ -741,19 +741,25 @@ void windows_request_context::do_response(bool bad_request) m_response = http::http_response(status_codes::InternalError); } - if (bad_request) - { - async_process_response(); - } - else + // Wait until the content download finished before replying because m_overlapped is reused, + // and we don't want to delete 'this' if the body is still downloading + pplx::create_task(proxy_content_ready).then([this](pplx::task t) { - // Wait until the content download finished before replying. - pplx::create_task(proxy_content_ready).then([this](pplx::task requestBody) + try { + t.wait(); async_process_response(); - }); - } + } + catch (...) + { + } + }).wait(); }); + + if (shouldWait == DontWaitForBody) + { + proxy_content_ready.set(); + } } void windows_request_context::async_process_response() From a1c52e57695fd61d189b379d96d5a3c720e3b6de Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Fri, 13 Nov 2015 13:52:09 +1100 Subject: [PATCH 03/10] Avoid another unobserved exception handler situation, and add an assert in an error case that looks problematic --- Release/src/http/listener/http_server_httpsys.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index a366641ae0..acfbb4f6d6 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -734,10 +734,18 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa // This means the user didn't respond to the request, allowing the // http_request instance to be destroyed. There is nothing to do then // so don't send a response. + // Avoid unobserved exception handler + pplx::create_task(proxy_content_ready).then([this](pplx::task t) + { + try { t.wait(); } catch(...) {} + }); return; } catch (...) { + // Should never get here, if we do there's a chance that a circular reference will cause leaks, + // or worse, undefined behaviour as we don't know who owns 'this' anymore + _ASSERTE(false); m_response = http::http_response(status_codes::InternalError); } From 11fa3ee5cb8a7664742af889a53057d5aec7a41d Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Fri, 13 Nov 2015 18:07:21 +1100 Subject: [PATCH 04/10] Move the callback that breaks the http_response circular reference so that it can't fire before the response claims ownership of the circular reference --- .../src/http/listener/http_server_httpsys.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index acfbb4f6d6..915a5ea0cc 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -465,13 +465,7 @@ void http_windows_server::receive_requests() pplx::task http_windows_server::respond(http::http_response response) { windows_request_context * p_context = static_cast(response._get_server_context()); - return pplx::create_task(p_context->m_response_completed).then([p_context](::pplx::task 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(); - }); + return pplx::create_task(p_context->m_response_completed); } windows_request_context::windows_request_context() @@ -711,6 +705,8 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa } catch (...) { + // Copy the request reference in case it's the last + http_request request = m_msg; m_msg = http_request(); proxy_content_ready.set_exception(std::current_exception()); cancel_request(std::current_exception()); @@ -749,6 +745,13 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa m_response = http::http_response(status_codes::InternalError); } + pplx::create_task(m_response_completed).then([this](::pplx::task t) + { + // After response is sent, break circular reference between http_response and the request context. + // Otherwise http_listener::close() can hang. + m_response._get_impl()->_set_server_context(nullptr); + }); + // Wait until the content download finished before replying because m_overlapped is reused, // and we don't want to delete 'this' if the body is still downloading pplx::create_task(proxy_content_ready).then([this](pplx::task t) From 0685728fd5934b86d259df21af16c341b9a71424 Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Fri, 13 Nov 2015 18:47:38 +1100 Subject: [PATCH 05/10] Work around leak of http_request when _complete is never called --- Release/src/http/listener/http_server_httpsys.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index 915a5ea0cc..37762d58a3 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -769,7 +769,8 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa if (shouldWait == DontWaitForBody) { - proxy_content_ready.set(); + // Fake a body completion so the content_ready() task doesn't keep the http_request alive forever + m_msg._get_impl()->_complete(0); } } From 90ebeb43f0dd1ceaa233fdbc375bb4b143737cba Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Fri, 13 Nov 2015 18:52:15 +1100 Subject: [PATCH 06/10] Fix accidental tab --- Release/src/http/listener/http_server_httpsys.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index 37762d58a3..e0ceaa840f 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -706,7 +706,7 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa catch (...) { // Copy the request reference in case it's the last - http_request request = m_msg; + http_request request = m_msg; m_msg = http_request(); proxy_content_ready.set_exception(std::current_exception()); cancel_request(std::current_exception()); From 3cb4da799a068daf1c9d436027fe49be3a958dd1 Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Thu, 19 Nov 2015 10:28:58 +1100 Subject: [PATCH 07/10] Fix another accidental tab indent --- Release/src/http/listener/http_server_httpsys.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index e0ceaa840f..be5f50b0e3 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -690,7 +690,7 @@ void windows_request_context::dispatch_request_to_listener(_In_ web::http::exper void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWait) { // Use a proxy event so we're not causing a circular reference between the http_request and the response task - pplx::task_completion_event proxy_content_ready; + pplx::task_completion_event proxy_content_ready; auto content_ready_task = m_msg.content_ready(); auto get_response_task = m_msg.get_response(); From 6c82566f047bbf37ec91354e46ed07c66922dff5 Mon Sep 17 00:00:00 2001 From: Tim Boundy Date: Wed, 25 Nov 2015 22:09:31 +1100 Subject: [PATCH 08/10] Update CONTRIBUTORS.txt --- CONTRIBUTORS.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index c1aa02bd51..a368135804 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -34,3 +34,6 @@ Cisco Systems Gergely Lukacsy (glukacsy) thomasschaub + +Trimble +Tim Boundy (gigaplex) \ No newline at end of file From 725c941d7af5564ded24fde12ffc63f288631d8a Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Wed, 2 Dec 2015 17:55:53 -0800 Subject: [PATCH 09/10] Enum classes are preferred to avoid implicit conversions --- Release/include/cpprest/details/http_server_httpsys.h | 6 +++--- Release/src/http/listener/http_server_httpsys.cpp | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Release/include/cpprest/details/http_server_httpsys.h b/Release/include/cpprest/details/http_server_httpsys.h index a97a461a8e..6ae9cc1e85 100644 --- a/Release/include/cpprest/details/http_server_httpsys.h +++ b/Release/include/cpprest/details/http_server_httpsys.h @@ -99,10 +99,10 @@ struct windows_request_context : http::details::_http_server_context // Dispatch request to the provided http_listener. void dispatch_request_to_listener(_In_ web::http::experimental::listener::details::http_listener_impl *pListener); - enum ShouldWaitForBody + enum class ShouldWaitForBody { - WaitForBody, - DontWaitForBody + Wait, + DontWait }; // Initialise the response task callbacks. If the body has been requested, we should wait for it to avoid race conditions. void init_response_callbacks(ShouldWaitForBody shouldWait); diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index be5f50b0e3..73b7f7ba6a 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -530,7 +530,7 @@ void windows_request_context::async_process_request(HTTP_REQUEST_ID request_id, { CancelThreadpoolIo(pServer->m_threadpool_io); m_msg.reply(status_codes::InternalError); - init_response_callbacks(DontWaitForBody); + init_response_callbacks(ShouldWaitForBody::DontWait); } } @@ -542,7 +542,7 @@ void windows_request_context::read_headers_io_completion(DWORD error_code, DWORD if(error_code != NO_ERROR) { m_msg.reply(status_codes::InternalError); - init_response_callbacks(DontWaitForBody); + init_response_callbacks(ShouldWaitForBody::DontWait); } else { @@ -578,7 +578,7 @@ void windows_request_context::read_headers_io_completion(DWORD error_code, DWORD m_msg.reply(status_codes::BadRequest, badRequestMsg); // Even though we have a bad request, we should wait for the body otherwise we risk racing over m_overlapped - init_response_callbacks(WaitForBody); + init_response_callbacks(ShouldWaitForBody::Wait); } } } @@ -654,7 +654,7 @@ void windows_request_context::dispatch_request_to_listener(_In_ web::http::exper // Save http_request copy to dispatch to user's handler in case content_ready() completes before. http_request request = m_msg; - init_response_callbacks(WaitForBody); + init_response_callbacks(ShouldWaitForBody::Wait); // Look up the lock for the http_listener. auto *pServer = static_cast(http_server_api::server_api()); @@ -767,7 +767,7 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa }).wait(); }); - if (shouldWait == DontWaitForBody) + if (shouldWait == ShouldWaitForBody::DontWait) { // Fake a body completion so the content_ready() task doesn't keep the http_request alive forever m_msg._get_impl()->_complete(0); From c966484b7e2683b3756153bfdc62679980376b06 Mon Sep 17 00:00:00 2001 From: Robert Schumacher Date: Wed, 2 Dec 2015 17:57:59 -0800 Subject: [PATCH 10/10] Minor cleanup --- Release/src/http/listener/http_server_httpsys.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Release/src/http/listener/http_server_httpsys.cpp b/Release/src/http/listener/http_server_httpsys.cpp index 73b7f7ba6a..72f7b1c069 100644 --- a/Release/src/http/listener/http_server_httpsys.cpp +++ b/Release/src/http/listener/http_server_httpsys.cpp @@ -708,8 +708,9 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa // Copy the request reference in case it's the last http_request request = m_msg; m_msg = http_request(); - proxy_content_ready.set_exception(std::current_exception()); - cancel_request(std::current_exception()); + auto exc = std::current_exception(); + proxy_content_ready.set_exception(exc); + cancel_request(exc); return; } @@ -731,9 +732,9 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa // http_request instance to be destroyed. There is nothing to do then // so don't send a response. // Avoid unobserved exception handler - pplx::create_task(proxy_content_ready).then([this](pplx::task t) + pplx::create_task(proxy_content_ready).then([](pplx::task t) { - try { t.wait(); } catch(...) {} + try { t.wait(); } catch (...) {} }); return; } @@ -745,7 +746,7 @@ void windows_request_context::init_response_callbacks(ShouldWaitForBody shouldWa m_response = http::http_response(status_codes::InternalError); } - pplx::create_task(m_response_completed).then([this](::pplx::task t) + pplx::create_task(m_response_completed).then([this](pplx::task t) { // After response is sent, break circular reference between http_response and the request context. // Otherwise http_listener::close() can hang.