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

XMLWordPrintable

## Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• 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

1. logTest2.cc
6 kB

## Activity

Hide
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
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
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
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
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
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
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
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
Andy Salnikov added a comment -

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

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

## People

• Assignee:
Andy Salnikov
Reporter:
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: