Skip to content

Fix connect timeout not accounting for DNS resolution time #810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ithewei merged 5 commits into from
Mar 11, 2026
Merged

Fix connect timeout not accounting for DNS resolution time #810

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e3a20d1
Initial plan
Copilot Mar 10, 2026
ea0d4a6
Fix connect timeout not accounting for DNS resolution time
Copilot Mar 10, 2026
1ac34d6
Address code review: use consistent error handling and fail immediate…
Copilot Mar 10, 2026
899eb0d
Use CHECK_TIMEOUT after connect and add hsocket.c+htime.c to nslookup…
Copilot Mar 11, 2026
d19257f
Add explicit closesocket(connfd) in ConnectTimeout before returning -…
Copilot Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ unittest: prepare
$(CXX) -g -Wall -O0 -std=c++11 -I. -Ibase -o bin/hthread_test unittest/hthread_test.cpp -pthread
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/hmutex_test unittest/hmutex_test.c base/htime.c -pthread
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/connect_test unittest/connect_test.c base/hsocket.c base/htime.c
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/socketpair_test unittest/socketpair_test.c base/hsocket.c
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/socketpair_test unittest/socketpair_test.c base/hsocket.c base/htime.c
$(CC) -g -Wall -O0 -std=c99 -I. -Iutil -o bin/base64 unittest/base64_test.c util/base64.c
$(CC) -g -Wall -O0 -std=c99 -I. -Iutil -o bin/md5 unittest/md5_test.c util/md5.c
$(CC) -g -Wall -O0 -std=c99 -I. -Iutil -o bin/sha1 unittest/sha1_test.c util/sha1.c
Expand All @@ -282,10 +282,10 @@ unittest: prepare
$(CXX) -g -Wall -O0 -std=c++11 -I. -Ibase -Icpputil -o bin/threadpool_test unittest/threadpool_test.cpp -pthread
$(CXX) -g -Wall -O0 -std=c++11 -I. -Ibase -Icpputil -o bin/objectpool_test unittest/objectpool_test.cpp -pthread
$(CXX) -g -Wall -O0 -std=c++11 -I. -Ibase -Issl -Ievent -Ievpp -Icpputil -Ihttp -Ihttp/client -Ihttp/server -o bin/sizeof_test unittest/sizeof_test.cpp
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -o bin/nslookup unittest/nslookup_test.c protocol/dns.c base/hsocket.c
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -o bin/nslookup unittest/nslookup_test.c protocol/dns.c base/hsocket.c base/htime.c
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -o bin/ping unittest/ping_test.c protocol/icmp.c base/hsocket.c base/htime.c -DPRINT_DEBUG
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -o bin/ftp unittest/ftp_test.c protocol/ftp.c base/hsocket.c
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -Iutil -o bin/sendmail unittest/sendmail_test.c protocol/smtp.c base/hsocket.c util/base64.c
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -o bin/ftp unittest/ftp_test.c protocol/ftp.c base/hsocket.c base/htime.c
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -Iutil -o bin/sendmail unittest/sendmail_test.c protocol/smtp.c base/hsocket.c base/htime.c util/base64.c

run-unittest: unittest
bash scripts/unittest.sh
Expand Down
10 changes: 9 additions & 1 deletion base/hsocket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "hsocket.h"

#include "hdef.h"
#include "htime.h"

#ifdef OS_UNIX
#include <poll.h>
Expand Down Expand Up @@ -339,9 +340,16 @@ int ConnectNonblock(const char* host, int port) {
}

int ConnectTimeout(const char* host, int port, int ms) {
unsigned int start_time = gettick_ms();
int connfd = Connect(host, port, 1);
if (connfd < 0) return connfd;
return ConnectFDTimeout(connfd, ms);
unsigned int elapsed = gettick_ms() - start_time;
int remaining = ms - (int)elapsed;
if (remaining <= 0) {
errno = ETIMEDOUT;
Copy link
Copy Markdown
Owner

@ithewei ithewei Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot 这里需要closesocket(connfd)吧？

Copy link
Copy Markdown
Contributor Author

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是的，已修复。改为显式调用 closesocket(connfd) 并直接返回 -ETIMEDOUT，不再依赖 socket_errno_negative()（在 Windows 上它读取 WSAGetLastError() 而非 errno，可能返回错误的错误码）。见 d19257f。

return socket_errno_negative(connfd);
}
return ConnectFDTimeout(connfd, remaining);
}

#ifdef ENABLE_UDS
Expand Down
25 changes: 19 additions & 6 deletions http/client/HttpClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ int http_client_connect(http_client_t* cli, const char* host, int port, int http
if (timeout > 0) {
blocktime = MIN(timeout*1000, blocktime);
}
unsigned int start_time = gettick_ms();
int connfd = ConnectTimeout(host, port, blocktime);
if (connfd < 0) {
hloge("connect %s:%d failed!", host, port);
Expand Down Expand Up @@ -241,7 +242,15 @@ int http_client_connect(http_client_t* cli, const char* host, int port, int http
if (!is_ipaddr(host)) {
hssl_set_sni_hostname(cli->ssl, host);
}
so_rcvtimeo(connfd, blocktime);
unsigned int elapsed = gettick_ms() - start_time;
int ssl_timeout = blocktime - (int)elapsed;
if (ssl_timeout <= 0) {
hssl_free(cli->ssl);
cli->ssl = NULL;
closesocket(connfd);
return NABS(ETIMEDOUT);
}
so_rcvtimeo(connfd, ssl_timeout);
int ret = hssl_connect(cli->ssl);
if (ret != 0) {
fprintf(stderr, "* ssl handshake failed: %d\n", ret);
Expand Down Expand Up @@ -321,6 +330,12 @@ static int http_client_exec(http_client_t* cli, HttpRequest* req, HttpResponse*
}
}

char recvbuf[1024] = {0};
char* data = NULL;
size_t len = 0;
int total_nsend, nsend, nrecv;
total_nsend = nsend = nrecv = 0;

if (connfd <= 0 || cli->host != req->host || cli->port != req->port) {
cli->host = req->host;
cli->port = req->port;
Expand All @@ -329,15 +344,13 @@ static int http_client_exec(http_client_t* cli, HttpRequest* req, HttpResponse*
if (connfd < 0) {
return connfd;
}
CHECK_TIMEOUT
}

cli->parser->SubmitRequest(req);
char recvbuf[1024] = {0};
int total_nsend, nsend, nrecv;
total_nsend = nsend = nrecv = 0;
send:
char* data = NULL;
size_t len = 0;
data = NULL;
len = 0;
while (cli->parser->GetSendData(&data, &len)) {
total_nsend = 0;
while (total_nsend < len) {
Expand Down
8 changes: 4 additions & 4 deletions unittest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ target_link_libraries(hmutex_test -lpthread)
add_executable(connect_test connect_test.c ../base/hsocket.c ../base/htime.c)
target_include_directories(connect_test PRIVATE .. ../base)

add_executable(socketpair_test socketpair_test.c ../base/hsocket.c)
add_executable(socketpair_test socketpair_test.c ../base/hsocket.c ../base/htime.c)
target_include_directories(socketpair_test PRIVATE .. ../base)

# ------util------
Expand Down Expand Up @@ -73,17 +73,17 @@ target_include_directories(objectpool_test PRIVATE .. ../base ../cpputil)
target_link_libraries(objectpool_test -lpthread)

# ------protocol------
add_executable(nslookup nslookup_test.c ../protocol/dns.c)
add_executable(nslookup nslookup_test.c ../protocol/dns.c ../base/hsocket.c ../base/htime.c)
target_include_directories(nslookup PRIVATE .. ../base ../protocol)

add_executable(ping ping_test.c ../protocol/icmp.c ../base/hsocket.c ../base/htime.c)
target_compile_definitions(ping PRIVATE -DPRINT_DEBUG)
target_include_directories(ping PRIVATE .. ../base ../protocol)

add_executable(ftp ftp_test.c ../protocol/ftp.c ../base/hsocket.c)
add_executable(ftp ftp_test.c ../protocol/ftp.c ../base/hsocket.c ../base/htime.c)
target_include_directories(ftp PRIVATE .. ../base ../protocol)

add_executable(sendmail sendmail_test.c ../protocol/smtp.c ../base/hsocket.c ../util/base64.c)
add_executable(sendmail sendmail_test.c ../protocol/smtp.c ../base/hsocket.c ../base/htime.c ../util/base64.c)
target_include_directories(sendmail PRIVATE .. ../base ../protocol ../util)
Comment on lines +83 to 87
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nslookup links only nslookup_test.c and dns.c, but dns.c calls WSAInit() under OS_WIN, which is implemented in base/hsocket.c. This target will fail to link on Windows. Consider adding ../base/hsocket.c (and ../base/htime.c, since hsocket.c now depends on gettick_ms) to the nslookup executable sources to keep CMake parity with the Makefile unittest build.

Copilot uses AI. Check for mistakes.

if(UNIX)
Expand Down