Merge lp:~epics-core/epics-base/ioc-shutdown into lp:~epics-core/epics-base/3.15
Status: | Rejected |
---|---|
Rejected by: | Andrew Johnson on 2014-07-23 |
Proposed branch: | lp:~epics-core/epics-base/ioc-shutdown |
Merge into: | lp:~epics-core/epics-base/3.15 |
Diff against target: |
1578 lines (+646/-80) 36 files modified
src/Makefile (+1/-1) src/ioc/as/asDbLib.c (+9/-0) src/ioc/as/asDbLib.h (+1/-0) src/ioc/db/Makefile (+2/-1) src/ioc/db/callback.c (+15/-2) src/ioc/db/callback.h (+1/-0) src/ioc/db/dbCa.c (+18/-7) src/ioc/db/dbCa.h (+1/-0) src/ioc/db/dbScan.c (+39/-4) src/ioc/db/dbScan.h (+1/-0) src/ioc/db/dbUnitTest.c (+123/-0) src/ioc/db/dbUnitTest.h (+54/-0) src/ioc/db/initHooks.c (+8/-0) src/ioc/db/initHooks.h (+1/-0) src/ioc/db/test/Makefile (+9/-0) src/ioc/db/test/dbShutdownTest.c (+94/-0) src/ioc/db/test/sRecord.db (+1/-0) src/ioc/misc/dbCore.dbd (+3/-0) src/ioc/misc/iocInit.c (+63/-8) src/ioc/misc/iocInit.h (+2/-0) src/libCom/as/asLib.h (+1/-0) src/libCom/as/asLibRoutines.c (+1/-2) src/libCom/error/errlog.c (+37/-20) src/libCom/error/errlog.h (+1/-0) src/libCom/iocsh/iocsh.cpp (+6/-4) src/libCom/misc/epicsExit.c (+39/-13) src/libCom/misc/epicsExit.h (+2/-1) src/libCom/osi/epicsThread.h (+3/-0) src/libCom/osi/os/RTEMS/osdThread.c (+9/-0) src/libCom/osi/os/WIN32/osdThread.c (+11/-0) src/libCom/osi/os/posix/osdThread.c (+19/-0) src/libCom/osi/os/vxWorks/osdThread.c (+13/-1) src/libCom/taskwd/taskwd.c (+23/-9) src/libCom/taskwd/taskwd.h (+1/-0) src/libCom/test/epicsExitTest.c (+21/-6) src/libCom/test/epicsThreadOnceTest.c (+13/-1) |
To merge this branch: | bzr merge lp:~epics-core/epics-base/ioc-shutdown |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Andrew Johnson | 2013-10-11 | Disapprove on 2014-07-23 | |
Review via email:
|
Description of the change
Add iocBuildNoCA() and iocShutdown() to iocInit API to allow running an IOC from withing a test program that is part of a test harness.
Doc changes in https:/
mdavidsaver (mdavidsaver) wrote : | # |
mdavidsaver (mdavidsaver) wrote : | # |
> static volatile enum ctl cbCtl;
Also, while I don't think this case will cause any problem, do we want to start using atomic ops for global flags like this as a matter of good practice?
mdavidsaver (mdavidsaver) wrote : | # |
Looks like re-init does quite work. Remaining issues include:
> Warning: Registration already done.
From the generated registrar code. Only a warning.
> Warning -- iocshRegisterVa
From iocsh.c. The lists for registered shell functions and variables need to be cleared.
> Access security is NOT enabled. Was asSetFilename specified before iocInit?
> iocBuild: asInit Failed.
The 'firstTime' flag (at least) in asDbLib.c also needs to be reset.
mdavidsaver (mdavidsaver) wrote : | # |
> Looks like re-init does quite work.
doesn't
- 12446. By mdavidsaver on 2013-10-11
-
dbShutdownTest
- 12447. By mdavidsaver on 2013-10-11
-
asShutdown
- 12448. By mdavidsaver on 2013-10-11
-
cleanup iocsh
- 12449. By mdavidsaver on 2013-10-11
-
cleanup dbScan
mdavidsaver (mdavidsaver) wrote : | # |
re-init crashes and errors are fixed (I think). Still leaking memory in dbReadCOM().
I've added a new test dbShutdownTest which does the complete startup and shutdown sequence twice. This is added in src/ioc/db/test despite the fact that it depends on src/std (ie base.dbd) because I'm too lazy to create src/std/test (or similar).
- 12450. By Ralph Lange on 2013-10-22
-
ioc/db/test: Add tests for common IOC threads to dbShutdownTest.c
Ralph Lange (ralph-lange) wrote : | # |
Seems that only the scan threads are restarted.
errlog, timerQueue, taskwd, and callback threads are missing in the second cycle().
Added a test that shows this.
- 12451. By Ralph Lange on 2013-10-22
-
ioc/db: Fix restart issue for callback threads.
- 12452. By Ralph Lange on 2013-10-22
-
libCom/error: Fix restart issue for errlog facility
- 12453. By Ralph Lange on 2013-10-22
-
libCom/taskwd: Fix restart issue.
- 12454. By Ralph Lange on 2013-10-22
-
ioc/db/test: Add testPlan number for dbShutdownTest
Ralph Lange (ralph-lange) wrote : | # |
Fixed the restart issues - the extended dbShutdownTests pass now.
- 12455. By Ralph Lange on 2013-10-22
-
ioc/misc: add missing include in iocInit.c
Andrew Johnson (anj) wrote : | # |
For future reference: With almost 2**10 lines of diff this branch mixes together (admittedly related) changes to several different subsystems, which makes it hard to review (Jeff's 3.15_libcom_
The documentation branch does not cover all the changes made here, it only talks about the modifications to the callback subsystem.
Is it wise to provide public APIs to all these Shutdown functions? I'm not so much concerned that this code doesn't work as whether by making them available we're letting ourselves in for a maintenance nightmare in the future. Do all these subsystems *need* to be shut down and restarted? What are the criteria for choosing which ones should and shouldn't be included? Could we shut everything down by calling epicsExitCallAt
The idea of resetting OnceFlags makes me cringe; they were never designed for that, and I'm not convinced that doing so is completely safe. The state change from EPICS_THREAD_
I'm not convinced that restarting an IOC inside the same process is a good idea, given the code we have (I know you're trying to change that, but I think it needs a properly planned solution, which this isn't [yet]). Just making the shutdown APIs available will mean that someone will want/try to use them on a running hard IOC, which is not going to work since we can't communicate the shutdown request to any device support or driver threads (other than by calling their epicsExitCallAt
IMHO if you need to test two independent IOCs, they need to be in two separate processes. You could split them into two test programs, or write the test as a Perl script which starts, controls and stops the IOCs as sub-processes; I have a Perl module which can do that on a host OS and could be extended to control an embedded IOC.
This isn't a definite NO, but I will need convincing that this is the right way to go.
- Andrew
mdavidsaver (mdavidsaver) wrote : | # |
On 12/03/2013 02:35 PM, Andrew Johnson wrote:
> For future reference: With almost 2**10 lines of diff this branch mixes together (admittedly related) changes to several different subsystems,
??? The merge proposal shows:
961 lines (+337/-53) 21 files modified
Also, the branch consists of many small, focused, changesets.
> The documentation branch does not cover all the changes made here, it only talks about the modifications to the callback subsystem.
Agreed, this needs to change.
> Is it wise to provide public APIs to all these Shutdown functions?
I would like to wrap all of this up into a single function (say
"testdbCleanup"). We could then make private header(s) for the many
shutdown calls. Would this make the change more acceptable.
> Could we shut everything down by calling epicsExitCallAt
This is an option I suppose.
> The idea of resetting OnceFlags makes me cringe;
A good point.
> I'm not convinced that restarting an IOC inside the same process is a good idea, given the code we have (I know you're trying to change that, but I think it needs a properly planned solution, which this isn't [yet]). Just making the shutdown APIs available will mean that someone will want/try to use them on a running hard IOC, which is not going to work since we can't communicate the shutdown request to any device support or driver threads (other than by calling their epicsExitCallAt
The goal is unittests. It it would help in your mind we can only allow
shutdown/cleanup between calling testPlan() and testDone().
Andrew Johnson (anj) wrote : | # |
On 12/03/2013 03:39 PM, mdavidsaver wrote:
> On 12/03/2013 02:35 PM, Andrew Johnson wrote:
>> For future reference: With almost 2**10 lines of diff this branch mixes
> together (admittedly related) changes to several different subsystems,
>
> ??? The merge proposal shows:
>
> 961 lines (+337/-53) 21 files modified
961 ≈ 1024
I'm just saying that the longer the diff is, the more reviewers will be
put off looking at it. If the changes were split into smaller dependent
branches, reviewing them might look like less work and could be done in
smaller chunks (this was a human psychology comment, not a programming
issue at all).
>> Is it wise to provide public APIs to all these Shutdown functions?
>
> I would like to wrap all of this up into a single function (say
> "testdbCleanup"). We could then make private header(s) for the many
> shutdown calls. Would this make the change more acceptable.
Hmm, maybe. How about an alternative API to iocInit, for test programs
only, and implemented outside of iocInit.c? I would be quite happy for
the various init*Sup() and related routines to be moved out of there and
into ioc/db somewhere, dbInit.c maybe?
>> Could we shut everything down by calling epicsExitCallAt
> and just ensure that each system's registered shutdown function leaves
> the subsystem in a state that can be properly restarted?
>
> This is an option I suppose.
The advantage of using epicsExitCallAt
expose the various Shutdown symbols at all, and we automatically stop
everything that we've started, in the (correct) reverse order.
The disadvantage is this will slow down IOC shutdown unnecessarily,
since we only need the 'clean up ready to start up again' actions in
those exit routines when we're running tests that need restart.
I'm getting visions of a subsystem management API; subsystems would
register both a fast exit and a slow clean-up routine with it. It could
even incorporate the taskwd, and provide subsystem/task status & health
info on request. This could also somewhere to centralize all of the
init/run/pause/exit enum task controls that are proliferating (which I
agree should probably become atomics).
Too much work?
- Andrew
--
Advertising may be described as the science of arresting the human
intelligence long enough to get money from it. -- Stephen Leacock
Ralph Lange (ralph-lange) wrote : | # |
> On 12/03/2013 03:39 PM, mdavidsaver wrote:
> > On 12/03/2013 02:35 PM, Andrew Johnson wrote:
> >> For future reference: With almost 2**10 lines of diff this branch mixes
> > together (admittedly related) changes to several different subsystems,
> >
> > ??? The merge proposal shows:
> >
> > 961 lines (+337/-53) 21 files modified
>
> 961 ≈ 1024
>
> I'm just saying that the longer the diff is, the more reviewers will be
> put off looking at it. If the changes were split into smaller dependent
> branches, reviewing them might look like less work and could be done in
> smaller chunks (this was a human psychology comment, not a programming
> issue at all).
I was considering this, but did not find a useful subset of the changes that would have added a functionality that could be documented and tested for.
Which in my imagination would have led to rejection of each individual branch and you commenting that a single functionality should have been presented as a single branch instead of multiple dependent branches that don't add anything useful by itself. ;-)
Admittedly, I might have been able to split the callback related things off. But still with a high change of not being able to add the mandatory tests.
Ralph Lange (ralph-lange) wrote : | # |
> The documentation branch does not cover all the changes made here, it only
> talks about the modifications to the callback subsystem.
If you are talking about the Application Developer's Guide, this change is IMHO the only one that is part of an API that Application Developers use.
Base development and internal APIs are not supposed to be covered by this document, correct?
Ralph Lange (ralph-lange) wrote : | # |
> The advantage of using epicsExitCallAt
> expose the various Shutdown symbols at all, and we automatically stop
> everything that we've started, in the (correct) reverse order.
>
> The disadvantage is this will slow down IOC shutdown unnecessarily,
> since we only need the 'clean up ready to start up again' actions in
> those exit routines when we're running tests that need restart.
>
> I'm getting visions of a subsystem management API; subsystems would
> register both a fast exit and a slow clean-up routine with it. It could
> even incorporate the taskwd, and provide subsystem/task status & health
> info on request. This could also somewhere to centralize all of the
> init/run/pause/exit enum task controls that are proliferating (which I
> agree should probably become atomics).
>
> Too much work?
If you think further, that becomes a major design change of iocCore.
It basically means going from somewhat modular spaghetti toward a component model where we have a mostly administrative core framework, and everything else becomes a component, possibly with separate versioning, providing a component API that handles component admin (version), compatibility (components requiring certain versions of other components), status/health, init/run/
The lists of available RSETs and DSETs would basically become extension points.
Existing support modules should have the choice of registering as a component. Plus maybe some generic (generated) code that handles existing record and device support to become (pseudo) components.
That would be a lot of work.
But I like the idea.
Andrew Johnson (anj) wrote : | # |
On 12/04/2013 03:49 AM, Ralph Lange wrote:
>> The documentation branch does not cover all the changes made here,
>> it only talks about the modifications to the callback subsystem.
>
> If you are talking about the Application Developer's Guide, this
> change is IMHO the only one that is part of an API that Application
> Developers use.
8.6.4 documents asInit(), so should mention asShutdown() nearby.
15.8.1 documents dbCaLinkInit(), so should mention dbCaShutdown(). Mea
culpa that it doesn't cover dbCaRun() or dbCaPause(), although they are
mentioned in section 7.
17.3 documents scanInit(), so should mention scanShutdown() nearby.
10.5 documents errlogInit(), so should mention errlogShutdown() nearby.
> Base development and internal APIs are not supposed to be covered by
> this document, correct?
Maybe, but as demonstrated above the documentation already covers
several internal APIs. If we don't modify those sections when we're
updating the code, the existing docs could start to contain wrong
information, which is a Bad Thing™ and may lead to Broken Window Syndrome.
--
Advertising may be described as the science of arresting the human
intelligence long enough to get money from it. -- Stephen Leacock
Andrew Johnson (anj) wrote : | # |
On 12/04/2013 04:56 AM, Ralph Lange wrote:
>> Too much work?
>
> If you think further, that becomes a major design change of iocCore.
I agree; maybe a Codeathon project for someone to look at?
> That would be a lot of work. But I like the idea.
I created a Blueprint on LP to keep the idea alive.
--
Advertising may be described as the science of arresting the human
intelligence long enough to get money from it. -- Stephen Leacock
Andrew Johnson (anj) wrote : | # |
So, where to go with this from here?
My preference would be to look further at the epicsExitCallAt
Andrew Johnson (anj) wrote : | # |
epicsExitCallAt
- 12456. By mdavidsaver on 2013-12-05
-
epicsThreadOnce
Reset - 12457. By mdavidsaver on 2013-12-05
-
use epicsThreadOnce
Reset - 12458. By mdavidsaver on 2013-12-05
-
cleanup initHook
- 12459. By mdavidsaver on 2013-12-05
-
add dbUnitTest.h
hide boilerplate of tests using the PDB
mdavidsaver (mdavidsaver) wrote : | # |
I've added epicsThreadOnce
Also I introduce dbUnitTest.h to provide the boilerplate which unittests running in an IOC should use.
- 12460. By Andrew Johnson on 2013-12-05
-
Make epicsExit subsystem reusable.
Calling epicsAtExit() after epicsExitCallAt
Exits() now
recreates the per-process list and registers the routine. - 12461. By mdavidsaver on 2013-12-14
-
deletePeriodic: fix list free
Can't iterate on free'd node
- 12462. By mdavidsaver on 2013-12-14
-
dbScan: avoid double shutdown
scanShutdown will be called (later) by iocShutdown
via exitDatabase() - 12463. By mdavidsaver on 2013-12-14
-
epicsExit: optional debug printing
Add a flag to cause a string to be printed
before each handler is run to show the order. - 12464. By mdavidsaver on 2013-12-14
-
errlog: nicer exit handler name
Ralph Lange (ralph-lange) wrote : | # |
Where are we on this?
I would like to get this branch into a mergeable state, but I have lost track a bit, and I'm not sure about the line between long-term goals and mandatory features for this to be merged.
Ralph Lange (ralph-lange) wrote : | # |
OK ... I think hiding the additional shutdown calls is what is left to do. I will
- move the various init*Sup() and related routines to ioc/db/dbInit.c
- create a private header dbInit.h
- call appropriately from iocInit.c and dbUnitTest.c through the dbInit.h API
- document dbUnitTest.h
That will not preclude us from doing more sophisticated module start/shutdown management later on, and be sufficient for the merge.
Correct?
Andrew Johnson (anj) wrote : | # |
Just merged the replacement ioc-shutdown2 branch, this merge is no longer applicable.
Unmerged revisions
- 12464. By mdavidsaver on 2013-12-14
-
errlog: nicer exit handler name
- 12463. By mdavidsaver on 2013-12-14
-
epicsExit: optional debug printing
Add a flag to cause a string to be printed
before each handler is run to show the order. - 12462. By mdavidsaver on 2013-12-14
-
dbScan: avoid double shutdown
scanShutdown will be called (later) by iocShutdown
via exitDatabase() - 12461. By mdavidsaver on 2013-12-14
-
deletePeriodic: fix list free
Can't iterate on free'd node
- 12460. By Andrew Johnson on 2013-12-05
-
Make epicsExit subsystem reusable.
Calling epicsAtExit() after epicsExitCallAt
Exits() now
recreates the per-process list and registers the routine. - 12459. By mdavidsaver on 2013-12-05
-
add dbUnitTest.h
hide boilerplate of tests using the PDB
- 12458. By mdavidsaver on 2013-12-05
-
cleanup initHook
- 12457. By mdavidsaver on 2013-12-05
-
use epicsThreadOnce
Reset - 12456. By mdavidsaver on 2013-12-05
-
epicsThreadOnce
Reset - 12455. By Ralph Lange on 2013-10-22
-
ioc/misc: add missing include in iocInit.c
Ralph, have you tested re-initialization? I see that callbackOnceFlag in callback.c is not being reset.