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

Branch/py3 7 mp queue close fix #4806

Open
wants to merge 2 commits into
base: branch/py3.7
Choose a base branch
from

Conversation

gitlab-importer
Copy link

In GitLab by @CrazyCasta on Jan 1, 2021, 05:14

This fixes #3372.

In short:

  • Current CPython multiprocessing.queues.Queue doesn't close properly unless put has been called before you try to close.
  • Not as simple as just changing the write pipe close as this has to be done in the context of the buffer thread.
  • This makes sure that before the thread has been started (before put has been called for instance) that something will close the write pipe.

See pypy issue #3372 and CPython issue #42752 for context.

This fixes the fact that close won't close the write pipe unless something had
already been put in the Queue.
@gitlab-importer gitlab-importer added the bug Something isn't working label Dec 30, 2023
@gitlab-importer
Copy link
Author

In GitLab by @mattip on Jan 1, 2021, 07:47

Something went wrong with the commit/merge request. It seems you targeted default instead of py3.7? Also why all the re-closing of issues? Your workflow should look something like

hg update py3.7  # or default if this is targeting py2.7
hg branch issue-3372
<add a failing test, fix code to make the test pass>
hg commit -m"test, fix for issue 3372: closing multiprocessing.queues.Queue"

Then push that single commit (or few commits) to heptapod
Then create a merge request against the target branch

@gitlab-importer
Copy link
Author

In GitLab by @mattip on Jan 1, 2021, 07:57

This is meant to be python2.7 only, so I think the problem is that you did not do the first hg update before adding your code, and then create a merge request from this branch off py3.7 to default.

@gitlab-importer
Copy link
Author

In GitLab by @CrazyCasta on Jan 6, 2021, 07:52

I'm pretty sure I did the update. I have no clue what all the closes business is, I would guess that's probably (hopefully) because I merged the wrong target. Let me take a look.

@gitlab-importer
Copy link
Author

In GitLab by @CrazyCasta on Jan 6, 2021, 07:52

changed target branch from branch/default to branch/py3.7

@gitlab-importer
Copy link
Author

In GitLab by @mattip on Jan 21, 2021, 13:39

The changeset looks good now. Could you add a #PyPy added to fix issue bpo 42752 to the test and code? They are part of the python stdlib which is managed by CPython, so it will help to resolve differences in future merges.

@gitlab-importer
Copy link
Author

In GitLab by @CrazyCasta on Mar 13, 2021, 20:13

added 1 commit

  • bcee2515 - Add PyPy added to fix tag.

Compare with previous version

@gitlab-importer
Copy link
Author

In GitLab by @alex-orange on Mar 13, 2021, 20:15

Sorry for the delay. I added one of those to each file. Wasn't sure how you want me to handle the queue.py file. There's changes peppered all over the place. I can add that comment around at each change if you'd like. At the moment it's just above the first big addition to the file.

@gitlab-importer
Copy link
Author

In GitLab by @mattip on Mar 15, 2021, 11:26

I'm not sure, but I don't think I want to deviate that far from the CPython implementation. Any idea why the cpython issue is not gaining traction there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants