You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/04/30 13:41:20 UTC

[GitHub] [thrift] emmenlau opened a new pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

emmenlau opened a new pull request #2127:
URL: https://github.com/apache/thrift/pull/2127


   This PR adds support for domain sockets for Windows. During the implementation I found a number of minor issues in TServerSocket and TSocket that I believe are improved in this PR. None of these changes should be a breaking change. Mostly some methods should not be called on a domain socket and have been excluded. Also, the variable `listening_` was set to `true` at the beginning of the `listen()` method but I found that this opens a small time window where `listening_` is true but the socket is not actually listening (yet).
   
   Please review and let me know if there is anything to discuss?
   
   I've tested this implementation successfully on MSVC 2017 and 2019, on Ubuntu 18.04 x86_64 and on MacOS 14 with XCode 11.3.
   
   - [x] I created Apache Jira Ticket https://issues.apache.org/jira/browse/THRIFT-5187
   - [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
   - [x] Did you squash your changes to a single commit?  (not required, but preferred)
   - [x] Did you do your best to avoid breaking changes?  If one was needed, did you label the Jira ticket with "Breaking-Change"?
   - [x] If your change does not involve any code, add ` [skip ci]` at the end of your pull request to free up build resources.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] stale[bot] commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-706785134


   This issue has been automatically closed due to inactivity. Thank you for your contributions.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] stale[bot] commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-666677845


   This issue is no longer stale. Thank you for your contributions.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] stale[bot] commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-703188598


   This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] stale[bot] closed pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #2127:
URL: https://github.com/apache/thrift/pull/2127


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] emmenlau commented on a change in pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
emmenlau commented on a change in pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#discussion_r575299225



##########
File path: lib/cpp/src/thrift/transport/TSocket.cpp
##########
@@ -700,6 +689,10 @@ int TSocket::getPort() {
   return port_;
 }
 
+std::string TSocket::getPath() {

Review comment:
       Thanks, fixed in latest PR.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] kainjow commented on a change in pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
kainjow commented on a change in pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#discussion_r432874696



##########
File path: lib/cpp/src/thrift/transport/TSocket.cpp
##########
@@ -700,6 +689,10 @@ int TSocket::getPort() {
   return port_;
 }
 
+std::string TSocket::getPath() {

Review comment:
       could this be `const`?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] emmenlau commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-621862664


   I took special care to make implement TSocket.cpp and TServerSocket very consistent. Where possible I reduced inconsistencies rather than adding any.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] emmenlau commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-730388975


   I've rebased this issue on latest master. Could someone please re-open so the changes become visible?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] emmenlau commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
emmenlau commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-730366544


   I'll rebase this on latest master. Dear stale bot, please reopen :-)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] kainjow commented on a change in pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
kainjow commented on a change in pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#discussion_r432874729



##########
File path: lib/cpp/src/thrift/transport/TSocket.cpp
##########
@@ -792,6 +789,13 @@ void TSocket::setKeepAlive(bool keepAlive) {
     return;
   }
 
+#ifdef _WIN32
+  if (!path_.empty()) {
+      // Windows Domain sockets do not support SO_KEEPALIVE.

Review comment:
       should this be a config.h setting instead?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] emmenlau commented on a change in pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
emmenlau commented on a change in pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#discussion_r575298703



##########
File path: build/cmake/ConfigureChecks.cmake
##########
@@ -52,14 +55,24 @@ check_include_file(sched.h HAVE_SCHED_H)
 check_include_file(string.h HAVE_STRING_H)
 check_include_file(strings.h HAVE_STRINGS_H)
 
+check_cxx_source_compiles(
+  "
+  #define WIN32_LEAN_AND_MEAN
+  #include <windows.h>
+  #include <winsock2.h>
+  #include <afunix.h>
+  int main(){(void)sizeof(((struct sockaddr_un *)0)->sun_path); return 0;}
+  "
+  HAVE_AF_UNIX_H)
+#check_struct_has_member("struct sockaddr_un" sun_path "afunix.h" HAVE_AF_UNIX_H LANGUAGE CXX)

Review comment:
       Thanks! Removed in latest PR.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] Jens-G commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-666677826


   .


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] stale[bot] commented on pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#issuecomment-665841548


   This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [thrift] kainjow commented on a change in pull request #2127: THRIFT-5187: Added Win32 support for domain sockets (AF_UNIX)

Posted by GitBox <gi...@apache.org>.
kainjow commented on a change in pull request #2127:
URL: https://github.com/apache/thrift/pull/2127#discussion_r432874427



##########
File path: build/cmake/ConfigureChecks.cmake
##########
@@ -52,14 +55,24 @@ check_include_file(sched.h HAVE_SCHED_H)
 check_include_file(string.h HAVE_STRING_H)
 check_include_file(strings.h HAVE_STRINGS_H)
 
+check_cxx_source_compiles(
+  "
+  #define WIN32_LEAN_AND_MEAN
+  #include <windows.h>
+  #include <winsock2.h>
+  #include <afunix.h>
+  int main(){(void)sizeof(((struct sockaddr_un *)0)->sun_path); return 0;}
+  "
+  HAVE_AF_UNIX_H)
+#check_struct_has_member("struct sockaddr_un" sun_path "afunix.h" HAVE_AF_UNIX_H LANGUAGE CXX)

Review comment:
       Is this commented-out line needed?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org