Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixup atexit handling #35

Merged
merged 3 commits into from
Sep 10, 2021
Merged

Conversation

mdavidsaver
Copy link
Contributor

  1. Add argument to epicsExit(int) to avoid exit with ~random code.
  2. Add call to atexit._run_exitfuncs() with py 3.x
  3. Cleanup/exit cleanly with interactive_ioc(call_exit=False)

allow interactive_ioc(call_exit=False) to cleanup
and exit cleanly.
@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #35 (797c3f6) into master (271481f) will decrease coverage by 0.24%.
The diff coverage is 75.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
- Coverage   85.74%   85.50%   -0.25%     
==========================================
  Files          13       13              
  Lines         807      828      +21     
==========================================
+ Hits          692      708      +16     
- Misses        115      120       +5     
Impacted Files Coverage Δ
softioc/softioc.py 87.27% <71.42%> (-1.62%) ⬇️
softioc/asyncio_dispatcher.py 90.90% <75.00%> (-9.10%) ⬇️
softioc/imports.py 96.22% <100.00%> (+0.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 271481f...797c3f6. Read the comment docs.

Copy link
Contributor

@thomascobb thomascobb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit hazy on the details of how we stop the IOC cleanly, so my comments reflect that...

def __call__(self):
safeEpicsExit()
def __repr__(self): # hack to exit when "called" with no parenthesis
sys.exit(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will make code.interact raise SystemExit which will call safeEpicsExit, Why do we call sys.exit() here rather than safeEpicsExit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sys.exit() will work the same regardless of how interactive_ioc() is called. This keeps the handling of call_exit=True vs. call_exit=False contained to interactive_ioc() where SystemExit is now caught.

@DiamondLightSource DiamondLightSource deleted a comment from thomascobb Sep 9, 2021
Copy link
Collaborator

@Araneidae Araneidae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me. Pity about that bug back in c9548f6!

Didn't know that EPICS had its own at exit framework; in fact, didn't know that EPICS handled exit at all now (apart from pulling the plug, like we always used to).

@@ -97,7 +97,11 @@ def auto_decode(result, func, args):
iocInit.argtypes = ()

epicsExit = Com.epicsExit
epicsExit.argtypes = ()
epicsExit.argtypes = (c_int,)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops.

Looks like this was my mistake, back in commit c9548f6

epicsExit()

elif hasattr(atexit, '_run_exitfuncs'): # py 3.x
atexit._run_exitfuncs()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do the same dance as above with sys.exitfunc to make sure we don't get multiple executions of exit handlers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as I read it. _run_exitfuncs() (like epicsExitCallAtExits()) removes callbacks which have run.

https://github.com/python/cpython/blob/06148b1870fceb1a21738761b8e1ac3bf654319b/Modules/atexitmodule.c#L105

def __repr__(self): # hack to exit when "called" with no parenthesis
sys.exit(0)
def __call__(self, code=0):
sys.exit(code)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we calling epicsExit here anymore?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you answered this above in #35 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully :)

@Araneidae
Copy link
Collaborator

@thomascobb, did I accidentally just delete one of your comments? I see a message to that effect upstream!

@mdavidsaver
Copy link
Contributor Author

Didn't know that EPICS had its own at exit framework ...

This has been around for quite some time. It's worth noting that there are some platform specific differences. epicsExitCallAtExits() is hooked into the libc atexit() on POSIX targets, and sysAtReboot() for vxWorks. However, this is not done for Windows due to differences in how process shutdown happens. On windows atexit() handlers run after all other threads have been summarily killed. So an epicsAtExit() handler trying to join one of these threads will deadlock.

epicsExit() is explicitly called from main() by soft IOCs to give uniformity. So I think it is reasonable to do the same when the python interpreter is shutting down, before eg. the GIL is cleaned up.

@Araneidae Araneidae merged commit 1e4f1e6 into DiamondLightSource:master Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants