Open vladimir-kovalev opened 8 years ago
vladimir-kovalev
commented
8 years ago And basically I think that no code inside the library (cpprestsdk) should implicitly call wait or get because there may be a situation when all available threads in a threadpool are locked (e.g. waiting) and no free thread exists to send a signal for those are waiting.
ras0219-msft
commented
8 years ago Generally, you're completely right about avoiding calls to wait or get -- however destructors are a bit more complex. In objects which control external asynchronous operations, the destructor needs to ensure all operations have completed before returning since otherwise you could very easily get undefined behavior.
This issue looks quite complex, I'm not immediately sure how to address it. One approach you could try is to manually call close() on the stream after the response is completed.
vladimir-kovalev
commented
8 years ago @ras0219-msft Thanks for the response. I thought about manually closing the stream but there's a problem: you only should close it when response is sent (an so you don't need the stream any more), but you can't (easily) say when it happens. reply() function just sets the response (sends a signal) and returns. Here's related code (Release/src/http/listener/http_listener_msg.cpp):
pplx::task<void> details::_http_request::_reply_impl(http_response response)
{
if (response.reason_phrase().empty())
{
response.set_reason_phrase(get_default_reason_phrase(response.status_code()));
}
pplx::task<void> response_completed;
#if !defined(__cplusplus_winrt) && _WIN32_WINNT >= _WIN32_WINNT_VISTA
... windows stuff ...
#endif
{
response_completed = pplx::task_from_result();
}
m_response.set(response);
return response_completed;
}I was not able to find in documentation (github pages/doxygen) or in the sources how to be notified when response is really sent.
Possible workaround - RAII class for response:
class http_response_holder: public web::http::http_response { // Or http_response can be a member
public:
~http_response_holder() { someResourceManager->readyToDestroy(body()); } // body() return concurrency::streams::istream
};But that solves only one issue and not another one: blocking operations. Suppose I have a function (like this StaticServer::serve) that serves the requests. Imagine this function does a blocking operation (like get on a task, e.g. when opening the stream, getting the body (extract_xxx), whatever). Consider N requests arrive at the same time and we have N threads in a thread pool. This function will be run in all N threads each serving its request. So we have a lock (well, not a deadlock, but sort of) again: all threads are waiting for a signal (some other thread(s) should do notify_xxx on a cond variable).
Possible solutions:
wait/get operations may spawn a couple of threads (until reaching hard limit) to solve that issue.void hostport_listener::start()
{
auto& service = crossplat::threadpool::shared_instance().service();
...
auto socket = new ip::tcp::socket(service);
...
}So we can add an option to http_listener_config - a maximum number of concurrent requests that server suppose to handle (e.g. 10 means no more than 10 concurrent requests allowed. If there are more they will need to wait). Using this option we can use a separate threadpool for each listener.
ahmedyarub
commented
5 years ago I'm reviving this issue because it's simply rendering the SDK useless. When the service is overwhelmed with many requests, in a short time all the threads would be in a status of waiting forever. The only way to solve this problem is be restarting the application. I've tested with Restinio (and other non-C++ frameworks) and did not face the problem. I'll be trying to make an implementation to solve this problem in the next days/weeks and share the results with you.
blockspacer
commented
4 years ago What's the current status of that issue?
jjmcwill
commented
3 years ago We've also been trying to limit the number of threads used by CPPRestSDK in our product due to the fact that some customers are sensitive to the amount of memory each worker thread consumes.
Unfortunately, that means all our worker threads (5) are deadlocked with the following stack:
`#0 0x00007fdfde741995 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
Sure seems similar. Has any work been done on this?
ahmedyarub
commented
3 years ago Just to make it clear: I'm not working on this one. I completely abandoned this framework.
jjmcwill
commented
3 years ago @ahmedyarub What did you switch to?
ahmedyarub
commented
3 years ago Restinio. I'm using it for my personal projects and tested only with benchmarks without being deployed to production.
Hi,
Either I do something wrong or there's a deadlock. I've implemented simple REST server using cpprestsdk. Basically it just serves a couple of endpoints and static content. Here's an implementation for static serving:
And here's how listeners are created:
I've restricted threadpool used in cpprestsdk from 40 to 4 threads because server is running on embedded device. Main server's thread calls
listen()and then just sleeps in an infinite loop. When I open the browser and send the request (justhttp://localhost:port/) it works for a couple of times and then locks. Here's the backtrace of one of the threads:All 4 threads in a threadpool are locked in this condition (all have the same backtrace). So am I doing something wrong? All 4 threads locked waiting for a signal. Here's the relevant code (
include/cpprest/filestream.h):So, all 4 threads are
waiting and no free threads are in threadpool to schedule a task which will fireset(this is just my guess). The~basic_file_bufferis called because I setconcurrency::streams::istreamas a body for response. So when response is destructed the stream is destructed too causing destruction of the underlying buffer.The server (and so cpprestsdk) is compiled and running on x86_64 linux (ubuntu 15.10). gcc version is 4.9.3. I can reproduce this in 100% of the cases. It serves the files for 2-3 times (sending around 20-30 files each time) and then locks.
Thanks in advance.