Skip to content

Commit 85d1265

Browse files
Copilotithewei
andauthored
Fix connect timeout not accounting for DNS resolution time (#810)
* Initial plan * Fix connect timeout not accounting for DNS resolution time The ConnectTimeout function previously only applied the timeout to the TCP connect phase (poll/select), but DNS resolution via getaddrinfo() had no timeout applied. On platforms like Android with no network, getaddrinfo() can block for 30-40 seconds, making user-set connect_timeout and timeout values ineffective. Changes: - ConnectTimeout now tracks elapsed time for DNS resolution and subtracts it from the TCP connect timeout. If DNS takes longer than the timeout, it returns ETIMEDOUT immediately. - http_client_connect adjusts SSL handshake timeout by subtracting time already spent on DNS+TCP connect. - http_client_exec checks overall request timeout after connect, handling cases where connect took longer than expected. - Updated build files (CMakeLists.txt, Makefile) to link htime.c with all targets that use hsocket.c. Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com> * Address code review: use consistent error handling and fail immediately on SSL timeout Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com> * Use CHECK_TIMEOUT after connect and add hsocket.c+htime.c to nslookup target - Replace inline timeout check with CHECK_TIMEOUT macro by moving variable declarations before the connect: label to avoid crossing initializations with goto. - Add hsocket.c and htime.c to nslookup CMake target for Windows link compatibility (dns.c calls WSAInit from hsocket.c). Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com> * Add explicit closesocket(connfd) in ConnectTimeout before returning -ETIMEDOUT When DNS resolution exhausts the timeout budget, explicitly close the socket with closesocket() and return -ETIMEDOUT directly, instead of relying on socket_errno_negative() which reads errno indirectly and could return the wrong error code on Windows. Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com>
1 parent 045fd61 commit 85d1265

4 files changed

Lines changed: 36 additions & 15 deletions

File tree

Makefile

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ unittest: prepare
268268
$(CXX) -g -Wall -O0 -std=c++11 -I. -Ibase -o bin/hthread_test unittest/hthread_test.cpp -pthread
269269
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/hmutex_test unittest/hmutex_test.c base/htime.c -pthread
270270
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/connect_test unittest/connect_test.c base/hsocket.c base/htime.c
271-
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/socketpair_test unittest/socketpair_test.c base/hsocket.c
271+
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -o bin/socketpair_test unittest/socketpair_test.c base/hsocket.c base/htime.c
272272
$(CC) -g -Wall -O0 -std=c99 -I. -Iutil -o bin/base64 unittest/base64_test.c util/base64.c
273273
$(CC) -g -Wall -O0 -std=c99 -I. -Iutil -o bin/md5 unittest/md5_test.c util/md5.c
274274
$(CC) -g -Wall -O0 -std=c99 -I. -Iutil -o bin/sha1 unittest/sha1_test.c util/sha1.c
@@ -282,10 +282,10 @@ unittest: prepare
282282
$(CXX) -g -Wall -O0 -std=c++11 -I. -Ibase -Icpputil -o bin/threadpool_test unittest/threadpool_test.cpp -pthread
283283
$(CXX) -g -Wall -O0 -std=c++11 -I. -Ibase -Icpputil -o bin/objectpool_test unittest/objectpool_test.cpp -pthread
284284
$(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
285-
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -o bin/nslookup unittest/nslookup_test.c protocol/dns.c base/hsocket.c
285+
$(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
286286
$(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
287-
$(CC) -g -Wall -O0 -std=c99 -I. -Ibase -Iprotocol -o bin/ftp unittest/ftp_test.c protocol/ftp.c base/hsocket.c
288-
$(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
287+
$(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
288+
$(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
289289

290290
run-unittest: unittest
291291
bash scripts/unittest.sh

base/hsocket.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#include "hsocket.h"
22

33
#include "hdef.h"
4+
#include "htime.h"
45

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

341342
int ConnectTimeout(const char* host, int port, int ms) {
343+
unsigned int start_time = gettick_ms();
342344
int connfd = Connect(host, port, 1);
343345
if (connfd < 0) return connfd;
344-
return ConnectFDTimeout(connfd, ms);
346+
unsigned int elapsed = gettick_ms() - start_time;
347+
int remaining = ms - (int)elapsed;
348+
if (remaining <= 0) {
349+
closesocket(connfd);
350+
return -ETIMEDOUT;
351+
}
352+
return ConnectFDTimeout(connfd, remaining);
345353
}
346354

347355
#ifdef ENABLE_UDS

http/client/HttpClient.cpp

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ int http_client_connect(http_client_t* cli, const char* host, int port, int http
211211
if (timeout > 0) {
212212
blocktime = MIN(timeout*1000, blocktime);
213213
}
214+
unsigned int start_time = gettick_ms();
214215
int connfd = ConnectTimeout(host, port, blocktime);
215216
if (connfd < 0) {
216217
hloge("connect %s:%d failed!", host, port);
@@ -241,7 +242,15 @@ int http_client_connect(http_client_t* cli, const char* host, int port, int http
241242
if (!is_ipaddr(host)) {
242243
hssl_set_sni_hostname(cli->ssl, host);
243244
}
244-
so_rcvtimeo(connfd, blocktime);
245+
unsigned int elapsed = gettick_ms() - start_time;
246+
int ssl_timeout = blocktime - (int)elapsed;
247+
if (ssl_timeout <= 0) {
248+
hssl_free(cli->ssl);
249+
cli->ssl = NULL;
250+
closesocket(connfd);
251+
return NABS(ETIMEDOUT);
252+
}
253+
so_rcvtimeo(connfd, ssl_timeout);
245254
int ret = hssl_connect(cli->ssl);
246255
if (ret != 0) {
247256
fprintf(stderr, "* ssl handshake failed: %d\n", ret);
@@ -321,6 +330,12 @@ static int http_client_exec(http_client_t* cli, HttpRequest* req, HttpResponse*
321330
}
322331
}
323332

333+
char recvbuf[1024] = {0};
334+
char* data = NULL;
335+
size_t len = 0;
336+
int total_nsend, nsend, nrecv;
337+
total_nsend = nsend = nrecv = 0;
338+
324339
if (connfd <= 0 || cli->host != req->host || cli->port != req->port) {
325340
cli->host = req->host;
326341
cli->port = req->port;
@@ -329,15 +344,13 @@ static int http_client_exec(http_client_t* cli, HttpRequest* req, HttpResponse*
329344
if (connfd < 0) {
330345
return connfd;
331346
}
347+
CHECK_TIMEOUT
332348
}
333349

334350
cli->parser->SubmitRequest(req);
335-
char recvbuf[1024] = {0};
336-
int total_nsend, nsend, nrecv;
337-
total_nsend = nsend = nrecv = 0;
338351
send:
339-
char* data = NULL;
340-
size_t len = 0;
352+
data = NULL;
353+
len = 0;
341354
while (cli->parser->GetSendData(&data, &len)) {
342355
total_nsend = 0;
343356
while (total_nsend < len) {

unittest/CMakeLists.txt

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ target_link_libraries(hmutex_test -lpthread)
2828
add_executable(connect_test connect_test.c ../base/hsocket.c ../base/htime.c)
2929
target_include_directories(connect_test PRIVATE .. ../base)
3030

31-
add_executable(socketpair_test socketpair_test.c ../base/hsocket.c)
31+
add_executable(socketpair_test socketpair_test.c ../base/hsocket.c ../base/htime.c)
3232
target_include_directories(socketpair_test PRIVATE .. ../base)
3333

3434
# ------util------
@@ -73,17 +73,17 @@ target_include_directories(objectpool_test PRIVATE .. ../base ../cpputil)
7373
target_link_libraries(objectpool_test -lpthread)
7474

7575
# ------protocol------
76-
add_executable(nslookup nslookup_test.c ../protocol/dns.c)
76+
add_executable(nslookup nslookup_test.c ../protocol/dns.c ../base/hsocket.c ../base/htime.c)
7777
target_include_directories(nslookup PRIVATE .. ../base ../protocol)
7878

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

83-
add_executable(ftp ftp_test.c ../protocol/ftp.c ../base/hsocket.c)
83+
add_executable(ftp ftp_test.c ../protocol/ftp.c ../base/hsocket.c ../base/htime.c)
8484
target_include_directories(ftp PRIVATE .. ../base ../protocol)
8585

86-
add_executable(sendmail sendmail_test.c ../protocol/smtp.c ../base/hsocket.c ../util/base64.c)
86+
add_executable(sendmail sendmail_test.c ../protocol/smtp.c ../base/hsocket.c ../base/htime.c ../util/base64.c)
8787
target_include_directories(sendmail PRIVATE .. ../base ../protocol ../util)
8888

8989
if(UNIX)

0 commit comments

Comments
 (0)