Client keep-alive support is broken #1041
Comments
|
Call stack leading to |
|
@gjasny, for clarification, does |
|
Any response please? |
|
Hello, I'm going to test ccache 4.4.1 which ignores the signal: ccache/ccache@b2a88e7 The test should be done tomorrow. Thanks for your patience, |
|
Even with ignoring SIGPIPE it's still broken: ccache/ccache#934 |
|
@gjasny, thanks for the additional comment. |
|
I'm having an issue where Post() returns an httplib::Result with a "Read" error code when using keep alive. It happens when doing a request with a long delay before it. My guess is that the server is terminating the connection and the HTTP lib is not picking up on it. Not sure if it's the same issue as here. |
|
@lightray22, I am not sure whether it's the same as @gjasny reported. Could you try to make the smallest possible code to reproduce it? It will tell the truth. Thanks! |
|
I think it is likely the same problem. I wrote a minimal program that produces a backtrace with valgrind. Ends with a SIGPIPE. Compile and run... Program output... I'm guessing that the server terminated the connection and httplib does not handle it. |
|
@lightray22, thanks for the detailed information! I now got two evidences. I am too busy to work on it though, hope I'll get back to this problem sometime this year. |
|
Also my compiler is |
|
Hello @yhirose
and I can confirm it really works for another issue I described in #1121 after you had fixed the original one. #ifndef CPPHTTPLIB_RECV_FLAGS
#define CPPHTTPLIB_RECV_FLAGS MSG_NOSIGNAL
#endif
#ifndef CPPHTTPLIB_SEND_FLAGS
#define CPPHTTPLIB_SEND_FLAGS MSG_NOSIGNAL
#endifand hope it's the correct way. |
|
@lightray22, could you take a look at the @tsilia's comment above. At least, the SIGPIPE issue can be handled accordingly. |
|
@gjasny, I finally started investigating this problem. As you mentioned, Line 5728 in 9639578 Line 2282 in 9639578 I thought Please let me know if you know a cross-platform way to detect a socket closed on the other side. I'll also do more research on it. Thanks for your help! |
This is probably because you only use write FDs in the mentioned Maybe you should consider trying this approach. |
|
@tsilia, thanks for the comment. But unless the client send ( |
|
Yes, I know the server woulnd't write to the socket on its end until we asked it to. |
|
Ok, this is kinda quick and dirty, but it actually works (I borrowed the socket-related code from you and someone else). You can try it yourself: void ClientThreadFunc(bool& bStop)
{
struct addrinfo hints, *res;
int sockfd;
memset(&hints, 0, sizeof hints);
hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_STREAM;
getaddrinfo(ServerAddress.c_str(), std::to_string(ServerPort).c_str(), &hints, &res);
sockfd = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
connect(sockfd, res->ai_addr, res->ai_addrlen);
while (!bStop)
{
fd_set fdsr;
FD_ZERO(&fdsr);
FD_SET(sockfd, &fdsr);
fd_set fdsw;
FD_ZERO(&fdsw);
FD_SET(sockfd, &fdsw);
timeval tv;
tv.tv_sec = static_cast<long>(0);
tv.tv_usec = static_cast<decltype(tv.tv_usec)>(0);
auto sel = select(static_cast<int>(sockfd + 1), &fdsr, nullptr, nullptr, &tv);
if (sel == 1)
{
if (FD_ISSET(sockfd, &fdsr))
{
char buf[1]{};
if (read(sockfd, &buf[0], sizeof(buf)) == 0)
{
std::cout << "Remote peer disconnected!" << std::endl;
break;
}
}
}
std::this_thread::sleep_for(1ms);
}
}
int main()
{
bool bStop = false;
std::thread t(&ClientThreadFunc, std::ref(bStop));
std::cout << "Press enter key to stop client..." << std::endl;
std::cin.get();
std::cerr << "* Stopping client..." << std::endl;
bStop = true;
if (t.joinable())
t.join();
std::cerr << "* Client stopped. Exiting..." << std::endl;
return 0;
}This code doesn't even need to write to socket anything, you can connect to a cpphttplib-powered server which has keep alive turned on and timeout set to, say, 5 seconds, and wait for the server to close socket. Once the server does so, the client will output 'Remote peer disconnected!'. Sorry if my comment was misleading or confusing. I probably should have mentioned that it's actually the TCP protocol that allows this possible, because when a peer closes the connection gracefully, there's some stuff going on between the peers at the TCP level. Detecting a lost connection (e.g., hung) is completely different story though. You will probably have to use TCP keep alive feature which can be turned on using SO_KEEPALIVE at SOL_SOCKET level. |
|
@yhirose I've fixed the previous post. |
|
@gjasny, @tsilia, I think I fixed the problem. Could you take a look at my change and unit test? If you don't find anything, I'll merge it. Thanks a lot for your help! |
|
I'll try to test it this week. Thanks, |
|
@yhirose The unit test is passing on my Linux machine (Ubuntu 20.04) But there's something wrong. But after the change the output is totally different; looks like now the same program recompiled with the I also commented in the pull request. UPD: I've checked with |
|
@tsilia, thanks for the quick code review. I updated is_socket_alive accordingly. Could you test again? |
|
@tsilia, thanks for all your reviews and suggestions for the pull request! It has finally been resolved. :) @gjasny, @lightray22, I would like to let you know that all the unit tests and @tsilia's test (https://github.com/tsilia/poc-httplib-inf-keepalive) passed successfully, and merged the pull request to the master. Please let me know if you find anything. Thanks. |
|
@yhirose For testing, I would suggest using @lightray22's code: |
|
Sorry for the delayed response... the new code works well for me also. Thanks! |
* Fix yhirose#1041 * Fixed problem with is_socket_alive * Adjust the way to check if the sockt is still alive. * Revert "Adjust the way to check if the sockt is still alive." This reverts commit 6c673b2. * Adjust is_socket_alive according to the code review
Hello,
I recently added this http library to server HTTP requests within the ccache compiler cache. Because a compilation requires up four HTTP requests I enabled keep-alive support in the Client.
But we observed a race condition on Linux:
Somehow the
is_alivedetection does not work as expected. A rough test case looks like this:Besides the non-working
is_alivedetection the library should set theMSG_NOSIGNALon Linux andSO_NOSIGPIPEon Apple platforms to not badly interfere with the library host process.Thanks,
Gregor
The text was updated successfully, but these errors were encountered: