Thrift
  1. Thrift
  2. THRIFT-1217

Use evutil_socketpair instead of pipe (Windows port)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.6.1, 0.7
    • Fix Version/s: 0.7
    • Component/s: C++ - Library
    • Labels:
    • Environment:

      Windows C++ 9.0, 10.0

    • Patch Info:
      Patch Available

      Description

      As part of an effort to use the Non-Blocking server (using libevent) on Windows, it was necessary to remove the use of "pipe" for the notification mechanism to signal end-of-task. We propose to use evutil_socketpair instead (tested with libevent 2.0.12). Patch included. Please see https://github.com/aubonbeurre/thrift/blob/alex-0.6.1/README.non.blocking.Windows for more details.

      1. thrift-1217.patchv0.2.txt
        7 kB
        alexandre parenteau
      2. evutil_socketpair.txt
        6 kB
        alexandre parenteau

        Activity

        Hide
        alexandre parenteau added a comment -

        Patch to teplace pipe by libevent's evutil_socketpair (based on thrift 0.6.1)

        Show
        alexandre parenteau added a comment - Patch to teplace pipe by libevent's evutil_socketpair (based on thrift 0.6.1)
        Hide
        Roger Meier added a comment -

        I really like to have Windows port within next Thrift release. The suggestion by David Reiss and me to use APR on Windows and do minimal changes on the current implementation is a key factor to bring this up and running.

        However, this patch depends on pthreads for windows and as mentioned on the dev mailing list or within Jira => it seems that this is not compatible with the apache license.

        Is APR feasible for your?

        Do you really need thet libevent version?
        Most GNU/Linux distro still provide 1.4.x and I would really like to keep capability to build and use thrift without lot of extra versions of libraries not within a standard distribution.

        Show
        Roger Meier added a comment - I really like to have Windows port within next Thrift release. The suggestion by David Reiss and me to use APR on Windows and do minimal changes on the current implementation is a key factor to bring this up and running. However, this patch depends on pthreads for windows and as mentioned on the dev mailing list or within Jira => it seems that this is not compatible with the apache license. Is APR feasible for your? Do you really need thet libevent version? Most GNU/Linux distro still provide 1.4.x and I would really like to keep capability to build and use thrift without lot of extra versions of libraries not within a standard distribution.
        Hide
        alexandre parenteau added a comment -

        @Roger: to avoid some confusion:

        • the attached patch does not depend on pthread. The suggestion here is merely to use evutil_socketpair instead of pipe.
        • evutil_socketpair is part of libevent 1.4 on ubuntu 10.10, although I did not test it.

        All the other changes related to win32 in the attached patch attached are not to take literally (Winsock2.h, static cast...).

        About THRIFT-1031:

        • my understanding is that THRIFT-1031 does not support async/libevent server on Windows (??)
        • libevent+thrift server seems VERY fast on win, and is needed for my project (blocking server won't do)
        • APR was not strictly necessary for the Non-Blocking server win32 port (i.e. libevent+pthread-win32), although I tried the THRIFT-1031 patch and observed it made the port a bit cleaner, so I would not refrain from adding APR to thrift-C++

        I decided after reviewing the win32 patches, it was better to open a separate issue, since the change here is atomic and should bare little consequences on linux. I was hoping that (naively) the win32 delta would get smaller by using the compatible call.

        All that said, I'd be really happy to contribute to THRIFT-1031 for it to go through. For this to happen, I would hope to have access to a shared implementation on top of 0.6.1 (the way I provided it on github), so I can test it regularly. Let me know if I can contribute somehow!

        As a conclusion, I think this patch is somewhat unrelated to THRIFT-1031, but it will help a future port of the NB server on win32 (assuming also someone will replace the pthread dependency by boost, which does not seem trivial at all).

        Show
        alexandre parenteau added a comment - @Roger: to avoid some confusion: the attached patch does not depend on pthread. The suggestion here is merely to use evutil_socketpair instead of pipe. evutil_socketpair is part of libevent 1.4 on ubuntu 10.10, although I did not test it. All the other changes related to win32 in the attached patch attached are not to take literally (Winsock2.h, static cast...). About THRIFT-1031 : my understanding is that THRIFT-1031 does not support async/libevent server on Windows (??) libevent+thrift server seems VERY fast on win, and is needed for my project (blocking server won't do) APR was not strictly necessary for the Non-Blocking server win32 port (i.e. libevent+pthread-win32), although I tried the THRIFT-1031 patch and observed it made the port a bit cleaner, so I would not refrain from adding APR to thrift-C++ I decided after reviewing the win32 patches, it was better to open a separate issue, since the change here is atomic and should bare little consequences on linux. I was hoping that (naively) the win32 delta would get smaller by using the compatible call. All that said, I'd be really happy to contribute to THRIFT-1031 for it to go through. For this to happen, I would hope to have access to a shared implementation on top of 0.6.1 (the way I provided it on github), so I can test it regularly. Let me know if I can contribute somehow! As a conclusion, I think this patch is somewhat unrelated to THRIFT-1031 , but it will help a future port of the NB server on win32 (assuming also someone will replace the pthread dependency by boost, which does not seem trivial at all).
        Hide
        Roger Meier added a comment -

        Yes, you are right. This is a libevent topic and not Windows Port relevant.

        I use libevent 1.4.13-stable-1 on Debian Squeeze and this version does not provide evutil_socket_t

        fixed that with the following define within lib/cpp/src/server/TNonblockingServer.h
        #define evutil_socket_t int

        one of the following defines should control the evutil_socket_t support, which one, which version?
        #define LIBEVENT_VERSION
        #define LIBEVENT_VERSION_NUMBER

        Could you provide a updated patch without windows headers and with an updated evutil_socket_t check rule?

        Show
        Roger Meier added a comment - Yes, you are right. This is a libevent topic and not Windows Port relevant. I use libevent 1.4.13-stable-1 on Debian Squeeze and this version does not provide evutil_socket_t fixed that with the following define within lib/cpp/src/server/TNonblockingServer.h #define evutil_socket_t int one of the following defines should control the evutil_socket_t support, which one, which version? #define LIBEVENT_VERSION #define LIBEVENT_VERSION_NUMBER Could you provide a updated patch without windows headers and with an updated evutil_socket_t check rule?
        Hide
        alexandre parenteau added a comment -

        Updated patch v0.2:

        • remove Windows stuff
        • use #ifdef HAVE_XXX
        • add cast macros
        Show
        alexandre parenteau added a comment - Updated patch v0.2: remove Windows stuff use #ifdef HAVE_XXX add cast macros
        Hide
        alexandre parenteau added a comment -

        @Roger: thanks for your input, I uploaded a new improved patch (thrift-1217.patchv0.2.txt). Tested compilation on Ubuntu with vanilla libevent 1.14.13, and latest libevent 2.0.12. Patch based on latest Thrift trunk.

        Show
        alexandre parenteau added a comment - @Roger: thanks for your input, I uploaded a new improved patch (thrift-1217.patchv0.2.txt). Tested compilation on Ubuntu with vanilla libevent 1.14.13, and latest libevent 2.0.12. Patch based on latest Thrift trunk.
        Hide
        Roger Meier added a comment -

        committed, thanks!

        Show
        Roger Meier added a comment - committed, thanks!
        Hide
        Hudson added a comment -

        Integrated in Thrift #186 (See https://builds.apache.org/job/Thrift/186/)
        THRIFT-1217 Use evutil_socketpair instead of pipe
        Patch: alexandre parenteau

        roger : http://svn.apache.org/viewvc/?view=rev&rev=1144286
        Files :

        • /thrift/trunk/lib/cpp/src/server/TNonblockingServer.h
        • /thrift/trunk/lib/cpp/src/server/TNonblockingServer.cpp
        Show
        Hudson added a comment - Integrated in Thrift #186 (See https://builds.apache.org/job/Thrift/186/ ) THRIFT-1217 Use evutil_socketpair instead of pipe Patch: alexandre parenteau roger : http://svn.apache.org/viewvc/?view=rev&rev=1144286 Files : /thrift/trunk/lib/cpp/src/server/TNonblockingServer.h /thrift/trunk/lib/cpp/src/server/TNonblockingServer.cpp
        Hide
        Hudson added a comment -

        Integrated in Thrift #245 (See https://builds.apache.org/job/Thrift/245/)
        THRIFT-1305. cpp: make TConnection a private inner class of
        TNonblockingServer

        The previous patch reverted some elements of THRIFT-1217. Fixed.

        Patch: Alexandre Parenteau

        bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1164161
        Files :

        • /thrift/trunk/lib/cpp/src/server/TNonblockingServer.cpp
        • /thrift/trunk/lib/cpp/src/server/TNonblockingServer.h
        Show
        Hudson added a comment - Integrated in Thrift #245 (See https://builds.apache.org/job/Thrift/245/ ) THRIFT-1305 . cpp: make TConnection a private inner class of TNonblockingServer The previous patch reverted some elements of THRIFT-1217 . Fixed. Patch: Alexandre Parenteau bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1164161 Files : /thrift/trunk/lib/cpp/src/server/TNonblockingServer.cpp /thrift/trunk/lib/cpp/src/server/TNonblockingServer.h

          People

          • Assignee:
            Unassigned
            Reporter:
            alexandre parenteau
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 24h
              24h
              Remaining:
              Remaining Estimate - 24h
              24h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development