-
Notifications
You must be signed in to change notification settings - Fork 5k
Added timeout for TCP calls such as connect, send, recv #3432
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.
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
Conversation
Now lets test on windows
Added default value to argument
|
@gabime sorry for the delay and not understanding the objective in the previous PR, but this time I have added a timeout to connect(), and also send() and recv() for tcp client. |
|
@gabime can I get some reviews on this PR ? So that I know if I should move forward. |
|
Sorry, I don’t have time to investigate it. Did you test it in both linux and windows? That the connect timeout works as expected? |
|
Yes I have tested on both linux and windows locally, the same can be proved from the appveyor CI pipeline. |
|
@gabime any updates on this PR ? |
|
I asked chatgpt to review. here are a few minor improvements it suggests for the windows impl: Always check return value of ioctlsocket (could fail). |
|
Will work on these today |
|
@gabime , I have made the changes you requested and also re-tested on Windows. |
|
You put some includes in middle of the code |
Changed improper misplaced includes.
|
I fixed that, please have a look |
|
Accroding to chatgpt: After getting WSAEWOULDBLOCK, you still must wait for the socket to become writable (i.e., the connection completes) using select() or WSAEventSelect() — and that is not done in the current code. Right now, the logic assumes: If connect() doesn’t return an immediate error other than WSAEWOULDBLOCK, then we're good to go. |
// Wait until socket is writable or timeout expires
fd_set wfds;
FD_ZERO(&wfds);
FD_SET(sockfd, &wfds);
rv = ::select(0, nullptr, &wfds, nullptr, const_cast<timeval *>(&tv));Arent we waiting for writability here ? |
|
Merged. Thanks @LowLevelLore |
|
You are welcome, it was quite an experience and learning.. |
* Now lets test on windows * I guess testing on windows passes. * Update tcp_client-windows.h Added default value to argument * Final edit * Update tcp_client-windows.h Changed improper misplaced includes.
Add optional timeout API to TCP connect, send, and recv calls
Worked on issue #2682
Overview
This pull request introduces an optional timeout parameter (
timeout_ms) to the existing TCP sink in spdlog, enabling users to specify a maximum duration for:By default,
timeout_ms = 0, preserving the original blocking behavior. Setting a non-zero value switches to a non-blocking socket withselect-based waiting.Key Changes
API Extension
tcp_client::connect(host, port, timeout_ms = 0)(both Windows and Linux implementations)timeout_ms = 0→ original blockingconnecttimeout_ms > 0→ non-blocking connect withselecttimeoutSend/Receive Timeouts
SO_SNDTIMEOandSO_RCVTIMEOare set totimeout_ms(on platforms supporting these).send(…)andrecv(…)calls will return with an error if they block longer than the specified duration.Configuration Struct
tcp_sink_configto include an optionaltimeout_msfield.tcp_sink_configwithouttimeout_mscontinues to default to blocking behavior.Cross-Platform Helper Function
connect_socket_with_timeout(...)for Windows, mirroring the Linux implementation.Usage