GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-96471: Add queue shutdown by EpicWink · Pull Request #96474 · python/cpython

Conversation

@EpicWink

@EpicWink EpicWink commented Sep 1, 2022 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Queue shutdown #96471

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@EpicWink
Add threading implementation of queue shutdown
4fd0640
@bedevere-bot

bedevere-bot commented Sep 1, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting review label Sep 1, 2022
EpicWink added 3 commits September 18, 2022 18:25
@EpicWink
Fix up implementation, add unit-tests
d942c9e
@EpicWink
Implement for asyncio queues
f552ac1
@EpicWink
WIP: multiprocessing queue shutdown
78671f9
@gvanrossum

gvanrossum commented Dec 1, 2022

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@EpicWink
WIP: multiprocessing queue shutdown
5f31f8e
@bedevere-bot bedevere-bot mentioned this pull request Jan 19, 2023
Queue shutdown #96471
Open
merwok
merwok reviewed Jan 19, 2023
View reviewed changes
Comment thread Lib/test/_test_multiprocessing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

YvesDup
YvesDup reviewed Feb 9, 2023
View reviewed changes
Comment thread Lib/asyncio/queues.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

YvesDup
YvesDup reviewed Feb 9, 2023
View reviewed changes
Comment thread Lib/test/test_asyncio/test_queues.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@arhadthedev arhadthedev added topic-asyncio stdlib Standard Library Python modules in the Lib/ directory labels Feb 9, 2023
@arhadthedev
Merge branch 'main' into queue-shutdown
a2035a6
YvesDup
YvesDup reviewed Feb 9, 2023
View reviewed changes
Comment thread Lib/asyncio/queues.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

YvesDup
YvesDup reviewed Feb 9, 2023
View reviewed changes
Comment thread Lib/queue.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

YvesDup
YvesDup reviewed Feb 9, 2023
View reviewed changes
Comment thread Lib/asyncio/queues.py Outdated
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:

@YvesDup YvesDup Feb 9, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
YvesDup
YvesDup reviewed Feb 9, 2023
View reviewed changes
Comment thread Lib/asyncio/queues.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

YvesDup
YvesDup reviewed Feb 10, 2023
View reviewed changes
Comment thread Lib/queue.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@YvesDup

YvesDup commented Feb 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@merwok

merwok commented Feb 10, 2023

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@YvesDup

YvesDup commented Feb 10, 2023

Copy link
Copy Markdown
Contributor

@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 ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@merwok

merwok commented Feb 10, 2023

Copy link
Copy Markdown
Member

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 and EpicWink reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@YvesDup

YvesDup commented Feb 12, 2023

Copy link
Copy Markdown
Contributor

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 ?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Feb 13, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@YvesDup

YvesDup commented Feb 13, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

EpicWink added 3 commits February 21, 2023 19:44
@EpicWink
Replace try-catch with assert-raises ctx-mgrs
978b8d1
@EpicWink
WIP: multiprocessing queue shut-down method
b19f529
@EpicWink
Use enum for asyncio queue shutdown state
6d3b3a5
EpicWink and others added 5 commits February 21, 2023 20:12
@EpicWink
Make asyncio queue state private
85827f1
@EpicWink
Replace try-catch with assert-raises ctx-mgrs
121de12
@EpicWink @YvesDup
Add asyncio queue tests
5e2df02
Co-authored-by: Duprat <[email protected]>
@EpicWink @YvesDup
Release joined awaitables
14c53d7
Co-authored-by: Duprat <[email protected]>
@EpicWink @YvesDup
Zero unfinished tasks
da10d9f
Co-authored-by: Duprat <[email protected]>
@EpicWink

EpicWink commented Feb 21, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@YvesDup YvesDup mentioned this pull request Mar 7, 2023
gh-96471: Add queue shutdown, next step. #102499
Closed
@YvesDup

YvesDup commented Mar 7, 2023

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mementum

This comment was marked as off-topic.

Sign in to view
willingc
willingc reviewed Apr 26, 2023
View reviewed changes

@willingc willingc left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@willingc willingc added awaiting changes and removed awaiting review labels Apr 26, 2023
This was referenced May 6, 2023
gh-96471: Add threading queue shutdown #104225
Closed
gh-96471: Add asyncio queue shutdown #104228
Merged
gh-96471: Add multiprocessing queue shutdown #104230
Draft
@EpicWink

EpicWink commented May 6, 2023

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

Reviewers

@merwok merwok merwok left review comments
@willingc willingc willingc left review comments
@gvanrossum gvanrossum gvanrossum left review comments
@gpshead gpshead Awaiting requested review from gpshead gpshead will be requested when the pull request is marked ready for review gpshead is a code owner
@1st1 1st1 Awaiting requested review from 1st1 1st1 will be requested when the pull request is marked ready for review 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov will be requested when the pull request is marked ready for review asvetlov is a code owner
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 will be requested when the pull request is marked ready for review kumaraditya303 is a code owner
+1 more reviewer
@YvesDup YvesDup YvesDup left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes stdlib Standard Library Python modules in the Lib/ directory topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@EpicWink @bedevere-bot @gvanrossum @YvesDup @merwok @mementum @willingc @arhadthedev
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.