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 — bpo-29704: Fix asyncio.SubprocessStreamProtocol closing by sethmlarson · Pull Request #405 · python/cpython

Conversation

@sethmlarson

@sethmlarson sethmlarson commented Mar 2, 2017

Copy link
Copy Markdown
Contributor

Was told by @1st1 to open this PR here from python/asyncio. See the original Pull Request for discussion and review: python/asyncio#485

Sorry, something went wrong.

Uh oh!

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

All reactions
@SethMichaelLarson
Add changes to subprocess.py from python/asycio #485
acd47e2
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 2, 2017
@mention-bot

mention-bot commented Mar 2, 2017

Copy link
Copy Markdown

@SethMichaelLarson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Haypo and @1st1 to be potential reviewers.

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Mar 2, 2017

Copy link
Copy Markdown
Member

LGTM. @asvetlov @fafhrd91 want to take a look too?

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Mar 2, 2017

Copy link
Copy Markdown
Member

@SethMichaelLarson Do we have an open issue for this on bugs.python.org? If not can you create one?

All reactions

Sorry, something went wrong.

Uh oh!

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

@SethMichaelLarson
Add test_read_sdout_after_process_exit from python/asyncio#485
d6c6567
@sethmlarson

sethmlarson commented Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

We do not, I will create one now.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sethmlarson

sethmlarson commented Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

I created an issue with @BotoX comments and links to issue on python/asyncio http://bugs.python.org/issue29704

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1 1st1 changed the title Pull Request from python/asyncio#485 bpo-29704: Fix asyncio.SubprocessStreamProtocol closing Mar 2, 2017
@SethMichaelLarson
Remove fd from _maybe_close_transport call
0372b36
@1st1

1st1 commented Mar 2, 2017

Copy link
Copy Markdown
Member

Please add a Misc/NEWS entry (to the top of Library section for 3.7.0 alpha)

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1 1st1 added needs backport to 3.5 labels Mar 2, 2017
@SethMichaelLarson
Add NEWS entry for bpo-29704
d7f619b
@sethmlarson

sethmlarson commented Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

@1st1 Done, is that entry acceptable?

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Mar 2, 2017

Copy link
Copy Markdown
Member

Yep. Looking good now.

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1
Merge branch 'master' into patch-1
90e84ea
@1st1
1st1 merged commit 481cb70 into python:master Mar 3, 2017
@1st1

1st1 commented Mar 3, 2017

Copy link
Copy Markdown
Member

This PR is merged, however, I didn't see that it had some trailing whitespace. Please make sure that your editor strips trailing whitespace.

All reactions

Sorry, something went wrong.

Uh oh!

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

1st1 pushed a commit to 1st1/cpython that referenced this pull request Mar 3, 2017
@SethMichaelLarson Yury Selivanov
bpo-29704: Fix asyncio.SubprocessStreamProtocol closing (python#405)
604faba
This was referenced Mar 3, 2017
bpo-29704: Fix asyncio.SubprocessStreamProtocol closing #415
Closed
bpo-29704: Fix asyncio.SubprocessStreamProtocol closing #416
Merged
@1st1

1st1 commented Mar 3, 2017

Copy link
Copy Markdown
Member

Note: tested this PR with all uvloop functional subprocess tests, everything looks alright.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sethmlarson

sethmlarson commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

@1st1 Sorry about that! My fault for not noticing. GitHub's web editor doesn't strip whitespace like PyCharm does. :) I'm glad it also works in uvloop, I should probably test asyncio changes with uvloop as well first before submitting.

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Mar 3, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I'm glad it also works in uvloop, I should probably test asyncio changes with uvloop as well first before submitting.

It's not so much about uvloop as it is about some unittests there that test both uvloop & vanilla asyncio. I plan to port some of those tests to CPython to increase coverage.

All reactions

Sorry, something went wrong.

Uh oh!

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

@1st1

1st1 commented Mar 3, 2017

Copy link
Copy Markdown
Member

Anyways, thanks for the contribution!

sethmlarson reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

@sethmlarson

sethmlarson commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

@1st1 Would be a good idea to port them, I've got some tests for selectors2 that don't occur in Python's stdlib selectors that might be considered good as well for unique situations.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sethmlarson
sethmlarson deleted the patch-1 branch March 3, 2017 04:51
@1st1

1st1 commented Mar 3, 2017

Copy link
Copy Markdown
Member

I've got some tests for selectors2 that don't occur in Python's stdlib selectors that might be considered good as well for unique situations.

Please open a PR with those tests, selectors module is very important. For an extra benefit, you'll see your tests running on the full CPython buildbots matrix.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sethmlarson

sethmlarson commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Will do, might have to experiment with different Python versions, PEP 475 really split the behavior for anything syscall-related in Python.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sethmlarson

sethmlarson commented Mar 3, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

What is the protocol for tests that behave differently (skip?) on different Python versions? Would they only be put in the version where they should be or is it normal to have tests that only run on Python 3.5+, for example.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 3, 2017 via email

Copy link
Copy Markdown
Member
Will do, might have to experiment with different Python versions, PEP 475
really split the behavior for anything syscall-related in Python. I lost track of asyncio. PEP 475 was implemented in Python 3.5. Python 3.4 doesn't accept bugfixes anymore. Why do you want to test different Python versions? Is asyncio on Python 3.3 or 3.4 still a thing?
All reactions

Sorry, something went wrong.

Uh oh!

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

@sethmlarson

sethmlarson commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

@Haypo Ah 3.4 is off bug-fix. Makes that a lot easier! :) Disregard my above comment then.
The tests would have to do with selectors.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 3, 2017 via email

Copy link
Copy Markdown
Member
We try to keep this table up to date: https://docs.python.org/devguide/#status-of-python-branches
All reactions

Sorry, something went wrong.

Uh oh!

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

@fafhrd91

fafhrd91 commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

@Haypo some rasberypi dev still on 3.4.2
https://home-assistant.io this is still on 3.4.2
I had to ship backported cookies module with aiohttp.

All reactions

Sorry, something went wrong.

Uh oh!

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

@vstinner

vstinner commented Mar 3, 2017 via email

Copy link
Copy Markdown
Member
@Haypo some rasberypi dev still on 3.4.2 https://home-assistant.io this is still on 3.4.2 I had to ship backported cookies module with aiohttp.
You are on the CPython project. There is also a http://github.com/python/asyncio project because there is no more release on http://pypi.python.org/pypi/asyncio/ since 2 years. I understand that asyncio doesn't evolve anymore on Python 3.4. Upgrade or backport yourself, no?
All reactions

Sorry, something went wrong.

Uh oh!

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

1st1 pushed a commit that referenced this pull request Mar 3, 2017
@SethMichaelLarson Yury Selivanov
bpo-29704: Fix asyncio.SubprocessStreamProtocol closing (#405)
f7f024a
@Mariatta Mariatta removed needs backport to 3.6 labels Mar 7, 2017
jaraco pushed a commit that referenced this pull request Dec 2, 2022
@zware @maxking
Use GH: rather than @ to tag core devs in Automerge-Triggered-By (#405)
334c817
* Use GH to signify Github handle of the user instead of "@".

"@" usually doesn't give any information about what exactly the handle means
since we don't know it is a Github handle from the commit message.

Changing the pattern to be `GH:` similar to what we do for Pull Requests
with (GH-).

Fixes #270

* Handle removing GH:-style Automerge-Triggered-By

...when automerge label is removed by a core-dev.

Co-authored-by: Abhilash Raj <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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

@sethmlarson @mention-bot @1st1 @vstinner @fafhrd91 @Mariatta @the-knights-who-say-ni @SethMichaelLarson
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.