Skip to content
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

Use TCP_KEEPALIVE only if TCP_KEEPIDLE is not defined #8539

Closed
wants to merge 1 commit into from

Conversation

MAntoniak
Copy link
Contributor

@MAntoniak MAntoniak commented Mar 4, 2022

The change based on a comment for TCP_KEEPALIVE.

I try using a CURLOPT_TCP_KEEPALIVE option. For TCP connections my application use lwIP library. The library defines both TCP_KEEPALIVE and TCP_KEEPIDLE. Both are responsible for the same thing, but TCP_KEEPALIVE is in milliseconds and TCP_KEEPIDLE is in seconds. After set TCP_KEEPIDLE correctly, TCP_KEEPALIVE overwrites changes. In the end, the application does not work as intended.

@dfandrich
Copy link
Collaborator

@dfandrich dfandrich commented Mar 4, 2022

I think a better approach would be to change this #ifdef section to use #elif so that in the end, only a single keepalive method is used. There's no reason any environment should need to set keepalive more than once, even if more than one method is available.

@MAntoniak
Copy link
Contributor Author

@MAntoniak MAntoniak commented Mar 4, 2022

I thought the same but I'm not familiar with Apple systems. Does it have TCP_KEEPIDLE defined? Won't #ifdef cause a malfunction on these systems?

@bagder
Copy link
Member

@bagder bagder commented Mar 4, 2022

I'm with @dfandrich and I would prefer the change to be something like this:

diff --git a/lib/connect.c b/lib/connect.c
index 64f951118..dd5277940 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -135,28 +135,27 @@ tcpkeepalive(struct Curl_easy *data,
     KEEPALIVE_FACTOR(optval);
     if(setsockopt(sockfd, IPPROTO_TCP, TCP_KEEPIDLE,
           (void *)&optval, sizeof(optval)) < 0) {
       infof(data, "Failed to set TCP_KEEPIDLE on fd %d", sockfd);
     }
+#elif TCP_KEEPALIVE
+    /* Mac OS X style */
+    optval = curlx_sltosi(data->set.tcp_keepidle);
+    KEEPALIVE_FACTOR(optval);
+    if(setsockopt(sockfd, IPPROTO_TCP, TCP_KEEPALIVE,
+          (void *)&optval, sizeof(optval)) < 0) {
+      infof(data, "Failed to set TCP_KEEPALIVE on fd %d", sockfd);
+    }
 #endif
 #ifdef TCP_KEEPINTVL
     optval = curlx_sltosi(data->set.tcp_keepintvl);
     KEEPALIVE_FACTOR(optval);
     if(setsockopt(sockfd, IPPROTO_TCP, TCP_KEEPINTVL,
           (void *)&optval, sizeof(optval)) < 0) {
       infof(data, "Failed to set TCP_KEEPINTVL on fd %d", sockfd);
     }
 #endif
-#ifdef TCP_KEEPALIVE
-    /* Mac OS X style */
-    optval = curlx_sltosi(data->set.tcp_keepidle);
-    KEEPALIVE_FACTOR(optval);
-    if(setsockopt(sockfd, IPPROTO_TCP, TCP_KEEPALIVE,
-          (void *)&optval, sizeof(optval)) < 0) {
-      infof(data, "Failed to set TCP_KEEPALIVE on fd %d", sockfd);
-    }
-#endif
 #endif
   }
 }
 
 static CURLcode

@MAntoniak MAntoniak changed the title Enable TCP_KEEPALIVE only for Apple devices Use TCP_KEEPALIVE only if TCP_KEEPIDLE is not defined Mar 4, 2022
@MAntoniak
Copy link
Contributor Author

@MAntoniak MAntoniak commented Mar 4, 2022

So let's do this way.

bagder
bagder approved these changes Mar 5, 2022
@bagder
Copy link
Member

@bagder bagder commented Mar 5, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants