dls-controls / pythonSoftIOC

Embed an EPICS IOC in a Python process
Apache License 2.0
32 stars 7 forks source link

AsyncioDispatcher cleanup tasks atexit #138

Closed mdavidsaver closed 3 months ago

mdavidsaver commented 11 months 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 11 months 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 11 months ago

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

Araneidae commented 11 months 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 11 months 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 11 months ago

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

AlexanderWells-diamond commented 11 months ago

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

mdavidsaver commented 4 months ago

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

coretl commented 4 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 4 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 3 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 3 months ago

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

AlexanderWells-diamond commented 3 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 3 months ago

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