Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-6989

ctrl_events/tests/EventAppenderTest.py fails Jenkins run-rebuild

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ctrl_events, log
    • Labels:
    • Story Points:
      3
    • Sprint:
      DB_F16_7
    • Team:
      Data Access and Database

      Description

      ctrl_events/tests/EventAppenderTest.py started failing on Jenkins "run-rebuild" last night:
      https://ci.lsst.codes/job/run-rebuild/354//console

      All test cases in EventAppenderTest.py did run and pass, but it failed with a Segmentation fault in the end.

      Jenkins "run-rebuild" uses a stack on NFS on lsst-dev (/nfs/home/lsstsw). The same test passes on regular Jenkins (stack-os-matrix).

        Attachments

          Issue Links

            Activity

            Hide
            smonkewitz Serge Monkewitz added a comment -

            Andy - assuming you mean getting rid of all globals in favor of function local static objects - how does that help? Static object destructors are run in reverse order of construction. So won't this bug surface anytime you have a construction order where activemqcpp objects get initialized after the logging objects? And won't that be provokable by importing lsst.log, doing some stuff to force log4cxx objects to get created, and then importing ctrl_events and executing the python code above?

            Outside of changing the way activemqcpp uses static objects, or moving the EventAppender from ctrl_events over to log, maybe you could force the log package to link against activemqcpp when it is available, or explicitly try loading activemqcpp at runtime before creating the log4cxx root logger?

            Also, I'm not sure if it is important, but ctrl_events seems to be missing a call to activemq::library::ActiveMQCPP::shutdownLibrary(), which seems to be present in the examples for that library. Then again, I don't know if it's safe to call outside of main, or how ctrl_events could arrange to call it at the right time.

            Show
            smonkewitz Serge Monkewitz added a comment - Andy - assuming you mean getting rid of all globals in favor of function local static objects - how does that help? Static object destructors are run in reverse order of construction. So won't this bug surface anytime you have a construction order where activemqcpp objects get initialized after the logging objects? And won't that be provokable by importing lsst.log, doing some stuff to force log4cxx objects to get created, and then importing ctrl_events and executing the python code above? Outside of changing the way activemqcpp uses static objects, or moving the EventAppender from ctrl_events over to log, maybe you could force the log package to link against activemqcpp when it is available, or explicitly try loading activemqcpp at runtime before creating the log4cxx root logger? Also, I'm not sure if it is important, but ctrl_events seems to be missing a call to activemq::library::ActiveMQCPP::shutdownLibrary(), which seems to be present in the examples for that library. Then again, I don't know if it's safe to call outside of main, or how ctrl_events could arrange to call it at the right time.
            Hide
            salnikov Andy Salnikov added a comment -

            Serge, you are right, my idea is to delay log4cxx initialization until ctrl_events is loaded. This indeed would not protect from a mess like:

            import lsst.log as log
            log.debug("log is imported!")
            import lsst.ctrl.events
            

            (there are more reasonable variations of this pattern of course)

            but it should work in most common case:

            import lsst.log as log
            import lsst.ctrl.events    # or other way round
             
            log.doStuff()
            

            I want to avoid "dependency" on activemqcpp in log package, this does not feel right and it does not scale if you want to use other appenders from other packages with similar issues.

            Making activemqcpp to shutdown could be a good option but I do not know if it is possible to know when it is the right moment to do so. Presumably this should be right before returning from Python script, but Python can terminate in unexpected ways too (with exception or sys.exit()). Still I do think some sort of protection would be nice to have in ctrl_events (one option I could think of is atexit() function which removes/destroys all appenders that were installed). Steve Pietrowicz, do you think something like this is doable?

            Show
            salnikov Andy Salnikov added a comment - Serge, you are right, my idea is to delay log4cxx initialization until ctrl_events is loaded. This indeed would not protect from a mess like: import lsst.log as log log.debug( "log is imported!" ) import lsst.ctrl.events (there are more reasonable variations of this pattern of course) but it should work in most common case: import lsst.log as log import lsst.ctrl.events # or other way round   log.doStuff() I want to avoid "dependency" on activemqcpp in log package, this does not feel right and it does not scale if you want to use other appenders from other packages with similar issues. Making activemqcpp to shutdown could be a good option but I do not know if it is possible to know when it is the right moment to do so. Presumably this should be right before returning from Python script, but Python can terminate in unexpected ways too (with exception or sys.exit()). Still I do think some sort of protection would be nice to have in ctrl_events (one option I could think of is atexit() function which removes/destroys all appenders that were installed). Steve Pietrowicz , do you think something like this is doable?
            Hide
            salnikov Andy Salnikov added a comment -

            @hchiang2, you are already familiar with the code, could you review my changes? This re-introduces the commit that you made on DM-6521 and improves initialization logic so that crash does not happen any more (in "normal" use cases). I have also tried to optimize several macros by avoiding duplicate calls to methods.

            Show
            salnikov Andy Salnikov added a comment - @hchiang2, you are already familiar with the code, could you review my changes? This re-introduces the commit that you made on DM-6521 and improves initialization logic so that crash does not happen any more (in "normal" use cases). I have also tried to optimize several macros by avoiding duplicate calls to methods.
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            Looks good to me. Thanks for finding the problem and fixing it, and other optimizations I missed in DM-6521.

            Show
            hchiang2 Hsin-Fang Chiang added a comment - Looks good to me. Thanks for finding the problem and fixing it, and other optimizations I missed in DM-6521 .
            Hide
            salnikov Andy Salnikov added a comment -

            Thanks for review! Merged and pushed to master. Done.

            Show
            salnikov Andy Salnikov added a comment - Thanks for review! Merged and pushed to master. Done.

              People

              • Assignee:
                salnikov Andy Salnikov
                Reporter:
                hchiang2 Hsin-Fang Chiang
                Reviewers:
                Hsin-Fang Chiang
                Watchers:
                Andy Salnikov, Greg Daues, Hsin-Fang Chiang, Serge Monkewitz, Steve Pietrowicz, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel