Qpid
  1. Qpid
  2. QPID-1982

QMF ResilientConnection notifyFd feature portability

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 0.5
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Windows

      Description

      The QMF's C++ qmfcommon ResilientConnection class has a setNotifyFd (int fd) method. The docs say this can receive a file descriptor that is written on when certain events take place, supposedly so the other end of the channel can listen, select, etc. and be notified of some event.

      This construct isn't portable, both because of the datatype of the fd (int is not necessarily a legit handle type everywhere) and because the code assumes a write() will work. The code can be abstracted out to get rid of the write() easily enough, but what is the usage for this facility? There are no calls to setNotifyFd() that I can find outside of the library containing the implementation. So I can't tell if this is something that can be replaced by qpid::sys::PollableCondition or not.

      What is the use case for this, and how can we make this portable? Different IPC type? Callback to some private hook?

        Issue Links

          Activity

          Steve Huston created issue -
          Hide
          Ted Ross added a comment -

          The interfaces in qpid/cpp/src/qmf are intended to be wrapped with bindings for various languages (Ruby, Python, C++, etc.). The language-specific wrapper (or driver) is responsible for determining what the threading model is for its API. The notifyFd can be abstracted but is has to be usable in a select/poll construct in any language.

          What is needed is a language-agnostic way of waking up a thread or process when it has work to do. It would be straight forward to replace "int" with a defined type and to replace "::write()" with a macro. I'm not sure, however, how the driver-side can be abstracted. It is not sufficient to have a blocking call that returns when there is work to do because the application may need to simultaneously be watching other sources of events (e.g. with select or poll).

          Show
          Ted Ross added a comment - The interfaces in qpid/cpp/src/qmf are intended to be wrapped with bindings for various languages (Ruby, Python, C++, etc.). The language-specific wrapper (or driver) is responsible for determining what the threading model is for its API. The notifyFd can be abstracted but is has to be usable in a select/poll construct in any language. What is needed is a language-agnostic way of waking up a thread or process when it has work to do. It would be straight forward to replace "int" with a defined type and to replace "::write()" with a macro. I'm not sure, however, how the driver-side can be abstracted. It is not sufficient to have a blocking call that returns when there is work to do because the application may need to simultaneously be watching other sources of events (e.g. with select or poll).
          Hide
          Steve Huston added a comment -

          Ok... so I assume that the other end of the pipe/socket now is some application unrelated to what's in subversion, yes?

          Would it be possible to replace the explicit use of an fd with a virtual interface that can be implemented out near the wrappers? Maybe the action of qmfcommon notifying is simply a method call through a defined interface. One possible implementation of the interface involves writing on an fd as is done now. And the wrapper would need to supply a pointer or reference to some object that conforms to the notification interface.

          Would that work?

          Show
          Steve Huston added a comment - Ok... so I assume that the other end of the pipe/socket now is some application unrelated to what's in subversion, yes? Would it be possible to replace the explicit use of an fd with a virtual interface that can be implemented out near the wrappers? Maybe the action of qmfcommon notifying is simply a method call through a defined interface. One possible implementation of the interface involves writing on an fd as is done now. And the wrapper would need to supply a pointer or reference to some object that conforms to the notification interface. Would that work?
          Hide
          Ted Ross added a comment -

          Another possibility is to use sys/PipeHandle.h which already has a windows implementation and appears to use "int" as its handle type. Does this make sense?

          Show
          Ted Ross added a comment - Another possibility is to use sys/PipeHandle.h which already has a windows implementation and appears to use "int" as its handle type. Does this make sense?
          Hide
          Steve Huston added a comment -

          I have a sneaking suspicion this issue is related to QPID-1918 and its fix for PipeHandle (eventually the lights do come on). Is the QMF plug-in on the other end of the notify fd?

          If so, then a workable short-term fix. combined with the PipeHandle fixes in QPID-1918, would be to change ResilientConnection's write() to send(). How does that look?

          In any event, I think this is going to cause long-term issues, at least on Windows. Although selecting a socket fd is a very common way to do such a notify, it's probably going to be limiting in some use cases down the road. Caveat emptor.

          Show
          Steve Huston added a comment - I have a sneaking suspicion this issue is related to QPID-1918 and its fix for PipeHandle (eventually the lights do come on). Is the QMF plug-in on the other end of the notify fd? If so, then a workable short-term fix. combined with the PipeHandle fixes in QPID-1918 , would be to change ResilientConnection's write() to send(). How does that look? In any event, I think this is going to cause long-term issues, at least on Windows. Although selecting a socket fd is a very common way to do such a notify, it's probably going to be limiting in some use cases down the road. Caveat emptor.
          Hide
          Steve Huston added a comment -

          I tried the send() change... it doesn't work because it requires the winsock header file be included, which is not cool in portable code.

          As I see it (which may be wrong) using PipeHandle wouldn't work since it would just create another set of handles. The ResilientConnection code is trying to use a handle from some other PipeHandle pair, yes?

          Show
          Steve Huston added a comment - I tried the send() change... it doesn't work because it requires the winsock header file be included, which is not cool in portable code. As I see it (which may be wrong) using PipeHandle wouldn't work since it would just create another set of handles. The ResilientConnection code is trying to use a handle from some other PipeHandle pair, yes?
          Steve Huston made changes -
          Field Original Value New Value
          Link This issue is related to QPID-1918 [ QPID-1918 ]
          Hide
          Steve Huston added a comment -

          QPID-1918 is proposing a workaround which is related to this. They should probably be looked at together.

          Show
          Steve Huston added a comment - QPID-1918 is proposing a workaround which is related to this. They should probably be looked at together.
          Steve Huston made changes -
          Link This issue is duplicated by QPID-2251 [ QPID-2251 ]
          Hide
          Alan Conway added a comment -

          We already have portable constructs for this type of notification.

          sys::PollableCondition is a low-level, portable mechanism that can be added to a Poller and allows you to wake a poller thread by setting the condition.

          sys::PollableQueue template is built on top of pollable condition. It allows dispatch to poller threads via an in-memory queue.

          I suspect PollableQueue is what's needed: replacing the use of ::write with a push to the pollable queue. If not a new portable mechansim can probably be built based on PollableCondition.

          Show
          Alan Conway added a comment - We already have portable constructs for this type of notification. sys::PollableCondition is a low-level, portable mechanism that can be added to a Poller and allows you to wake a poller thread by setting the condition. sys::PollableQueue template is built on top of pollable condition. It allows dispatch to poller threads via an in-memory queue. I suspect PollableQueue is what's needed: replacing the use of ::write with a push to the pollable queue. If not a new portable mechansim can probably be built based on PollableCondition.
          Hide
          Steve Huston added a comment -

          The PollableQueue idea sounds promising, Alan. QMF folks, does this work for you?

          Show
          Steve Huston added a comment - The PollableQueue idea sounds promising, Alan. QMF folks, does this work for you?
          Hide
          Ian Main added a comment -

          I agree it should be changed. If nothing else the entire thing should be handled by the API. eg instead of setNotifyFd, we should be calling notifyEngine() and it should do all the work of how to handle the queues etc. Also in the wrapper we can have a waitForEvents(). This could still use the unix pipe if that's appropriate, and I'm sure windows has a similar thing, but it should all be hidden in the C++ regardless of the implementation.

          Show
          Ian Main added a comment - I agree it should be changed. If nothing else the entire thing should be handled by the API. eg instead of setNotifyFd, we should be calling notifyEngine() and it should do all the work of how to handle the queues etc. Also in the wrapper we can have a waitForEvents(). This could still use the unix pipe if that's appropriate, and I'm sure windows has a similar thing, but it should all be hidden in the C++ regardless of the implementation.
          Hide
          Ian Main added a comment -

          I just realized that moving away from a file descriptor would make life difficult for ruby. Ruby does not use pthreads, it uses its own threading mechanism that includes using select() for a ruby read() call so that if it blocks it can run another thread. If the ruby thread blocks in a C++ library, the whole ruby process will block. Not sure how to deal with this yet.

          Show
          Ian Main added a comment - I just realized that moving away from a file descriptor would make life difficult for ruby. Ruby does not use pthreads, it uses its own threading mechanism that includes using select() for a ruby read() call so that if it blocks it can run another thread. If the ruby thread blocks in a C++ library, the whole ruby process will block. Not sure how to deal with this yet.
          Hide
          Ian Main added a comment -

          Using a file descriptor also allows QMF to incorporate itself into other mainloop providors, such as GTK/gnome. In a GTK/gnome app, GTK expects to have you sitting in its main loop waiting for events. Using an FD allows us to add another fd to that mainloop and receive callbacks when it becomes ready for reading. Also, gtk/gnome do this and do it on windows as well so I'm sure it's possible. We might need a portability layer there however.

          Show
          Ian Main added a comment - Using a file descriptor also allows QMF to incorporate itself into other mainloop providors, such as GTK/gnome. In a GTK/gnome app, GTK expects to have you sitting in its main loop waiting for events. Using an FD allows us to add another fd to that mainloop and receive callbacks when it becomes ready for reading. Also, gtk/gnome do this and do it on windows as well so I'm sure it's possible. We might need a portability layer there however.

            People

            • Assignee:
              Unassigned
              Reporter:
              Steve Huston
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Development