DiamondLightSource / pythonSoftIOC

Embed an EPICS IOC in a Python process
Apache License 2.0
31 stars 9 forks source link

AsyncioDispatcher cleanup tasks atexit #138

Closed mdavidsaver closed 5 months ago

mdavidsaver commented 1 year ago

Currently, the atexit logic simply breaks the event loop leaving an in-progress Task incomplete. Instead, use asyncio.run(), which cancels tasks before returning (among other cleanup).

Motivated by usage of asyncio.create_subprocess_exec() where the present behavior leaves the child process running. Similarly, for cleanup of temporary files/directories.

The downside is that asyncio.run() is introduced in python 3.7. So the 3.6 tests will fail. Emulating asyncio.run() in 3.6 would be possible, but is it worth the effort?

I don't know what is going wrong with the windows and mac tests.

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.15%. Comparing base (d55483d) to head (f7a1f53).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #138 +/- ## ========================================== + Coverage 87.73% 88.15% +0.41% ========================================== Files 14 14 Lines 1019 1038 +19 ========================================== + Hits 894 915 +21 + Misses 125 123 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

AlexanderWells-diamond commented 1 year ago

I have no attachment to Python 3.6 so am happy to see it go.

Araneidae commented 1 year ago

@coretl, I also have no ties to 3.6 ... except I would point out that it is the stock version of Python installed on RHEL8, so dropping it may have consequences for users on that platform.

mdavidsaver commented 1 year ago

... stock version of Python installed on RHEL8 ...

This is a fair point. I can think of two possibilities in addition to drop support for 3.6 or a full backport of asyncio.run.

State that the asyncio part is only supported with py >= 3.7. (fyi. driven by removal of loop= P4P only supports asyncio with py >= 3.6, but supports thread/cothread with py 2.7 and >= 3.4)

Continue the current behavior with <= 3.6

coretl commented 1 year ago

My preference is to drop 3.6, bump the major release number, and see if anyone complains... Any objections?

AlexanderWells-diamond commented 1 year ago

No objections here as long as its clear in the Changelog.

mdavidsaver commented 6 months ago

Is there anything I can do to move this PR forward?

coretl commented 6 months ago

Is there anything I can do to move this PR forward?

If you can drop 3.6 from the GitHub actions tests matrix, then add a test with your usecase (checking a spawned subprocess running as a Task exits cleanly outside context manager) then I'd be happy to merge this.

@AlexanderWells-diamond @Araneidae any thoughts?

Araneidae commented 6 months ago

I have learnt that there is a python3.11 rpm available on our RHEL8 channel and so presumably potentially available on all administered RHEL8 systems, so I agree that 3.6 can now go.

AlexanderWells-diamond commented 6 months ago

This PR will need some tidying up before we can merge it (remove 3.6, documentation, changelog etc.); I'll do that in the near future.

AlexanderWells-diamond commented 5 months ago

I've updated this PR to remove Python 3.6 support and added a Changelog entry.

AlexanderWells-diamond commented 5 months ago

With my addition of a test for the Context Manager, I'm finished with my changes. @Araneidae and/or @coretl please merge when you are happy with it.

mdavidsaver commented 5 months ago

@AlexanderWells-diamond Thank you for taking the time to finish this up!