Qpid
  1. Qpid
  2. QPID-2367

Early Initialization of File Descriptors Conflicts With Daemon Best Practices

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.5
    • Fix Version/s: 0.19
    • Component/s: C++ Client
    • Labels:
      None
    • Environment:

      Linux (possibly all UNIX), c+, g+

      Description

      At least one file descriptor (in qpid/sys/epoll/EpollPoller.*) in the c++ client is global and declared as static. In programs linked against the c++ qpid libs g++ generates code for allocation and, more importantly, initialization of these descriptors that occurs before main(). You can confirm this with gdb by breakpointing both the initialization and main() (the initialization break is hit first).

      On the other hand, the canonical recipe for creating a UNIX daemon calls for the closing of all open file descriptors after fork()ing (where the fork() certainly occurs after main()). While not an absolute requirement, closing all open file descriptors is considered a best practice. A loop to close all descriptors is also common in boilerplate daemon creation code and has undoubtedly been cut-and-pasted into numerous daemons.

      The net effect is that the typical daemon will close the file descriptor opened before main() in the c++ client library. In the case of the epoll code this manifests as an inability to connect to the broker.

      A fix for this would be to defer the initialization of the file descriptor (perhaps via the Singleton pattern or a move of the variables into a class member).

        Activity

        Hide
        Andrew Stitcher added a comment -

        It's curious we don't see this in qpidd when run as a daemon, I guess we're not using the "best practices"

        Show
        Andrew Stitcher added a comment - It's curious we don't see this in qpidd when run as a daemon, I guess we're not using the "best practices"
        Hide
        Andrew Stitcher added a comment -

        I think the fix most in line with the current code would be to use a singleton pattern.

        However when you add all the possibilities of forking into the mix of how this code is used I wonder if this would catch all the cases.

        For instance what about a process that uses amqp then forks a daemon and then uses amqp in the forked daemon? Is that a meaningful scenario? If it is you'd run into the same issue again. Which implies that although it appears wasteful a class member might be needed.

        Show
        Andrew Stitcher added a comment - I think the fix most in line with the current code would be to use a singleton pattern. However when you add all the possibilities of forking into the mix of how this code is used I wonder if this would catch all the cases. For instance what about a process that uses amqp then forks a daemon and then uses amqp in the forked daemon? Is that a meaningful scenario? If it is you'd run into the same issue again. Which implies that although it appears wasteful a class member might be needed.
        Hide
        Andrew Stitcher added a comment -

        Obviously, as I imply in my first comment, there is a simple work around, which is not to close all the fds before forking to daemonise.

        I think that you really only need to close 0,1,2 when daemonising anyway (just to make sure you don't have a stdin,stdout,stderr) and reopen them as /dev/null. More important is creating a new processs group so you don't get terminal signals.

        When we daemonise qpidd we only close 0,1,2 and reopen as /dev/null and call setsid(). That I believe is minimally required, anything else is not.

        Show
        Andrew Stitcher added a comment - Obviously, as I imply in my first comment, there is a simple work around, which is not to close all the fds before forking to daemonise. I think that you really only need to close 0,1,2 when daemonising anyway (just to make sure you don't have a stdin,stdout,stderr) and reopen them as /dev/null. More important is creating a new processs group so you don't get terminal signals. When we daemonise qpidd we only close 0,1,2 and reopen as /dev/null and call setsid(). That I believe is minimally required, anything else is not.
        Hide
        Jason Schlauch added a comment -

        Yes, you don't need to handle anything other than stdout, stdin, and stderr. However, for whatever reason, boilerplate daemon code often suggests (and goes on to implement) closing all file descriptors. So perhaps "best practice" wasn't quite accurate, and "common practice" or "it was in the code you inherited" might be better.

        I handled this by simply changing the daemon code to close only stdout, stdin, and stderr. Finding the error, however, took up a fair chunk of time. Some defensive coding in the QPID client might save others the same trouble.

        For example, could you simply check if the fd was open before attempting to use it (and reopen it if it's not)?

        Show
        Jason Schlauch added a comment - Yes, you don't need to handle anything other than stdout, stdin, and stderr. However, for whatever reason, boilerplate daemon code often suggests (and goes on to implement) closing all file descriptors. So perhaps "best practice" wasn't quite accurate, and "common practice" or "it was in the code you inherited" might be better. I handled this by simply changing the daemon code to close only stdout, stdin, and stderr. Finding the error, however, took up a fair chunk of time. Some defensive coding in the QPID client might save others the same trouble. For example, could you simply check if the fd was open before attempting to use it (and reopen it if it's not)?
        Hide
        Andrew Stitcher added a comment -

        This should be fixed now on trunk with r1368685

        Show
        Andrew Stitcher added a comment - This should be fixed now on trunk with r1368685

          People

          • Assignee:
            Andrew Stitcher
            Reporter:
            Jason Schlauch
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development