Skip to content

gh-96471: Add queue shutdown #96474

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

Closed
wants to merge 14 commits into from
Closed

Conversation

EpicWink
Copy link
Contributor

@EpicWink EpicWink commented Sep 1, 2022

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@gvanrossum
Copy link
Member

Note that once this is ready for review we may need three separate reviewers -- one for asyncio, another for multiprocessing, a third for threading.

@bedevere-bot bedevere-bot mentioned this pull request Jan 19, 2023
@arhadthedev arhadthedev added topic-asyncio stdlib Python modules in the Lib dir labels Feb 9, 2023
@@ -113,6 +131,8 @@ async def put(self, item):
Put an item into the queue. If the queue is full, wait until a free
slot is available before adding item.
"""
if self.shutdown_state != _queue_alive:
Copy link
Contributor

@YvesDup YvesDup Feb 9, 2023

Choose a reason for hiding this comment

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

an example:

Suggested change
if self.shutdown_state != _queue_alive:
if self.shutdown_state is not _QueueState.ALIVE:

@YvesDup
Copy link
Contributor

YvesDup commented Feb 10, 2023

  1. I have 6 more tests for file Lib/test/test_queue.py . How to proceed to send you all these lines ? Sorry for my question but I am not familiar with git commands :-(
  2. @EpicWink I do not see any shutdown method in file Lib/multiprocessing/queues.py . Can you confirm please ? But There is a close method.
    UPDATE:
    self._shutdown_state attr must be add to tuples of __setstate__ and __getstate__ in order to be included in serialization. Refers to lines 67 & 71 of Lib/multiprocessing/queues.py

@merwok
Copy link
Member

merwok commented Feb 10, 2023

This should help: https://devguide.python.org/getting-started/git-boot-camp/

The basic commands you will need are git add Lib/test/some/file to register changes in a file, git commit to save the registered changes with a note, git push to send the commits to your repository. The pull request will then be updated automatically.

@YvesDup
Copy link
Contributor

YvesDup commented Feb 10, 2023

@merwok Thank for your feedback. I created my own branch from EpicWink:queue-shutdown, and I am wondering how to give access to my changes. So may be @EpicWink can merge from YvesDup:queue-shutdown ?

@merwok
Copy link
Member

merwok commented Feb 10, 2023

Oh I missed that you weren’t the author of the PR!

Yes, you can make a pull request from your fork to epicwink’s work, or use the suggestion system here (comment on a line, find suggestion in the toolbar) to paste your new tests.

@YvesDup
Copy link
Contributor

YvesDup commented Feb 12, 2023

I read again the original topic and found nothing about what to do about the 'joined' tasks, threads (or processes). I just complete the shutdown method (only in asyncio and threading) to release them. May be it is not a good option, or a sufficient option. Should we raise a ShutDownexception for each of them ? Thoughts ?

@gvanrossum
Copy link
Member

@YvesDup What is your definition of a "'joined' task or thread"?

@YvesDup
Copy link
Contributor

YvesDup commented Feb 13, 2023

@gvanrossum: Task (or thread) that has called the .join() method of Queue. It is blocked until (_)unfinished_task attribute of Queue drops to 0. "blocked" task is more appropriate in that case.

@EpicWink
Copy link
Contributor Author

I'm reviewing @YvesDup 's changes in EpicWink#2

@YvesDup
Copy link
Contributor

YvesDup commented Mar 7, 2023

I have just created a new PR gh-96471: Add queue shutdown, next step. to resume and continue working on this feature

@mementum

This comment was marked as off-topic.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

@YvesDup Would you like to close this PR and split up #102499 into 3 PRs? Ideally breaking out asyncio into its own PR. Thanks!

@EpicWink
Copy link
Contributor Author

EpicWink commented May 6, 2023

This has been superseded by #102499, then by #104225, #104228 and #104230

@EpicWink EpicWink closed this May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes stdlib Python modules in the Lib dir topic-asyncio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants