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

Drop Windows event loop hack in Python 3.8+ #34

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Sep 5, 2023

In Python versions prior to 3.8, it appears that attempting to close the event loop after a Ctrl-C would reliably hang the process, which would need to be killed.

I've been unable to reproduce the behavior in any newer Python versions, so I think it's time to set the timeline for removing the hack entirely.

This should take care of the common ResourceWarning messages when using newer Python versions.

This change was already made in colcon/colcon-core#573 and colcon/colcon-core#581.

@cottsay cottsay added the bug Something isn't working label Sep 5, 2023
@cottsay cottsay self-assigned this Sep 5, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.14% 🎉

Comparison is base (32ef9ac) 80.91% compared to head (c8162ad) 81.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   80.91%   81.06%   +0.14%     
==========================================
  Files           2        2              
  Lines         131      132       +1     
  Branches       40       41       +1     
==========================================
+ Hits          106      107       +1     
  Misses         16       16              
  Partials        9        9              
Files Changed Coverage Δ
colcon_parallel_executor/executor/parallel.py 80.91% <100.00%> (+0.14%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

In Python versions prior to 3.8, it appears that attempting to close the
event loop after a Ctrl-C would reliably hang the process, which would
need to be killed.

I've been unable to reproduce the behavior in any newer Python versions,
so I think it's time to set the timeline for removing the hack entirely.

This should take care of the common ResourceWarning messages when using
newer Python versions.
@cottsay cottsay changed the title Drop ResourceWarning suppression Drop Windows event loop hack in Python 3.8+ Sep 6, 2023
@cottsay cottsay marked this pull request as ready for review September 6, 2023 21:50
Copy link

@Crola1702 Crola1702 left a comment

Choose a reason for hiding this comment

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

As far as I understand, this is kind of a bugfix for Python < 3.8 versions, right?

@cottsay
Copy link
Member Author

cottsay commented Sep 13, 2023

As far as I understand, this is kind of a bugfix for Python < 3.8 versions, right?

I'd call it a bugfix for Python >= 3.8 actually. Earlier than that, we still can't reliably call close() on the loop without hanging the process, but in Python >= 3.8, we seem to be able to do so and it also started producing a ResourceWarning letting us know that we forgot to close the loop.

There should be no change in behavior for Python < 3.8.

@cottsay cottsay merged commit d5dc9fe into master Sep 13, 2023
17 checks passed
@delete-merged-branch delete-merged-branch bot deleted the cottsay/resource-warning branch September 13, 2023 15:39
@cottsay cottsay added this to the 0.2.5 milestone Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants