GeezerVison

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

GeezerVison is showing github.com — Delay closing SubprocessStreamProtocol's Transport until all pipes are closed. by sethmlarson · Pull Request #485 · python/asyncio

This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Conversation

sethmlarson commented Jan 13, 2017

Copy link

Copy Markdown

I believe this change fixes #484.

Delays the closing for SubprocessStreamProtocol._transport until all pipes have been closed and the process has exited rather than just the process exiting. This allows for reading data from the subprocess that is still pending in the pipe after the process has exited.

I haven't run tests or created a test for this exact issue but I will create a few tests later.

Sorry, something went wrong.

Uh oh!

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

BotoX reacted with thumbs up emoji

All reactions

👍 1 reaction


          Delay SubprocessStreamProtocol closing transport

c73df72

Delays the closing for `SubprocessStreamProtocol._transport` until all pipes have been closed and the process has exited rather than just the process exiting. This allows for reading data from the subprocess that is still pending in the pipe after the process has exited.

the-knights-who-say-ni added the CLA signed label

Jan 13, 2017

sethmlarson mentioned this pull request

Jan 13, 2017

subprocess._loop deleted too soon causing exception when trying to read from pipe #484

Open

sethmlarson commented Jan 18, 2017

Copy link

Copy Markdown

Author

As pretty much the sole author of SubprocessStreamProtocol I think you're the best to review this @Haypo. (If you don't want to be pinged in the future let me know! :) )

All reactions

Sorry, something went wrong.

Uh oh!

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

1st1 commented Jan 18, 2017

Copy link

Copy Markdown

Member

cc @asvetlov. Although without a test there's nothing to review :)

All reactions

Sorry, something went wrong.

Uh oh!

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

sethmlarson commented Jan 18, 2017

Copy link

Copy Markdown

Author

@1st1 Yes that's true, it'd be nice to get a nod to say this is actually a sane thing to do though. Doesn't break any other tests but I don't know what other untested assumptions I might be breaking! :P

All reactions

Sorry, something went wrong.

Uh oh!

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

1st1 commented Jan 18, 2017

Copy link

Copy Markdown

Member

Yes that's true, it'd be nice to get a nod to say this is actually a sane thing to do though

It looks sane.

Asking for a review without tests isn't productive though, because you're essentially asking someone to stop doing what they are doing, understand (or remember, doesn't matter) your patch and all code that it touches, and make an opinion if your patch works. PRs without tests are usually reviewed the last if at all.

All reactions

Sorry, something went wrong.

Uh oh!

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


          Add simple test for patch

12428a2

sethmlarson commented Jan 18, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Author

Sorry, won't do it again. I added a simple test that fails on old build and passes with the proposed change. I presume it's okay to have a test that doesn't assert anything, just to make sure that no errors are thrown? The test is taken pretty much 1-to-1 with the attached issue.

I'm sure I can come up with a few more tests if more tests are required. :)

All reactions

Sorry, something went wrong.

Uh oh!

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

BotoX commented Feb 7, 2017

Copy link

Copy Markdown

I can confirm that this pull request fixes my issue #484
No more exceptions and no data is lost when the process exits, thanks! 👍

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.


          Remove comment about /dev/urandom

33cd1c5

1st1 reviewed

Mar 2, 2017

View reviewed changes

Comment thread

asyncio/subprocess.py Outdated

		def _maybe_close_transport(self, fd):
		if fd in self._pipe_fds:
		self._pipe_fds.remove(fd)
		if len(self._pipe_fds) == 0:

1st1 Mar 2, 2017

Copy link

Copy Markdown

Member

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.

Shouldn't this if len(...) be dedented?

Sorry, something went wrong.

Uh oh!

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

All reactions

sethmlarson Mar 2, 2017

Copy link

Copy Markdown

Author

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.

I guess that's true. My logic is we know that if nothing gets removed it won't be closed.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Allow _maybe_close_transport to close even if...

16a04d2

...removal from fd list does not occur.

1st1 reviewed

Mar 2, 2017

View reviewed changes

Comment thread

asyncio/subprocess.py Outdated

		self._limit = limit
		self.stdin = self.stdout = self.stderr = None
		self._transport = None
		self._pipe_fds = [-1]

1st1 Mar 2, 2017

Copy link

Copy Markdown

Member

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.

What does -1 mean here?

Sorry, something went wrong.

Uh oh!

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

All reactions

sethmlarson Mar 2, 2017

Copy link

Copy Markdown

Author

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.

I should probably add a comment. -1 is just the token for "process_exited" meaning we won't close a Transport until the process exits and all the pipes are done.

Sorry, something went wrong.

Uh oh!

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

All reactions

sethmlarson Mar 2, 2017

Copy link

Copy Markdown

Author

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.

Should I use a better sentinel object than -1?

Sorry, something went wrong.

Uh oh!

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

All reactions

1st1 Mar 2, 2017

Copy link

Copy Markdown

Member

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.

Ah, interesting. I'd add a separate boolean attribute for that.

Sorry, something went wrong.

Uh oh!

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

All reactions

sethmlarson Mar 2, 2017

Copy link

Copy Markdown

Author

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.

Sure thing!

Sorry, something went wrong.

Uh oh!

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

All reactions

1st1 reviewed

Mar 2, 2017

View reviewed changes

Comment thread

asyncio/subprocess.py Outdated

		reader.feed_eof()
		else:
		reader.set_exception(exc)
		self._maybe_close_transport(fd)

1st1 Mar 2, 2017

Copy link

Copy Markdown

Member

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.

I'd simply always call this method in pipe_connection_lost. Makes the code simpler to follow, and the logic with if statements should be (as they currently are) in _maybe_close_transport.

Sorry, something went wrong.

Uh oh!

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

All reactions

sethmlarson Mar 2, 2017

Copy link

Copy Markdown

Author

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.

Sure, just dedent that line then?

Sorry, something went wrong.

Uh oh!

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

All reactions

1st1 Mar 2, 2017

Copy link

Copy Markdown

Member

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.

yep.

Sorry, something went wrong.

Uh oh!

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

All reactions

sethmlarson Mar 2, 2017

Copy link

Copy Markdown

Author

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.

Alright, just wanted to make sure I was understanding that comment correctly. :)

Sorry, something went wrong.

Uh oh!

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

All reactions


          Add flag for process_exited, separate this from...

dc23e3f

..._pipe_fds and change interface of _maybe_close_transport to not take an fd.

1st1 commented Mar 2, 2017

Copy link

Copy Markdown

Member

Can you please close this PR and make a new one to http://github.com/python/cpython (with a link to this discussion and review). It would make merging this way easier for me.

All reactions

Sorry, something went wrong.

Uh oh!

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

sethmlarson commented Mar 2, 2017

Copy link

Copy Markdown

Author

So I made another change to _maybe_close_transport() to accommodate the flag for process exit.
Removed the fd parameter (since process exit isn't -1 anymore) and moved the removal into the pipe_connection_lost.

All reactions

Sorry, something went wrong.

Uh oh!

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

sethmlarson commented Mar 2, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Author

@1st1 Oh, sure! Can I do this when I'm home from work? Don't have my actual environment all setup here, making do with the GitHub web interface. :)

All reactions

Sorry, something went wrong.

Uh oh!

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

sethmlarson mentioned this pull request

Mar 2, 2017

bpo-29704: Fix asyncio.SubprocessStreamProtocol closing python/cpython#405

Merged

1st1 commented Mar 2, 2017

Copy link

Copy Markdown

Member

I like it now. Thanks for opening the new PR! I'll close this one.

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.

1st1 closed this

Mar 2, 2017

sethmlarson deleted the patch-1 branch

March 2, 2017 22:25

Reviewers

1 more reviewer

1st1

1st1 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

CLA signed

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.

5 participants

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.

Conversation

sethmlarson commented Jan 13, 2017

Uh oh!

sethmlarson commented Jan 18, 2017

Uh oh!

1st1 commented Jan 18, 2017

Uh oh!

sethmlarson commented Jan 18, 2017

Uh oh!

1st1 commented Jan 18, 2017

Uh oh!

sethmlarson commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BotoX commented Feb 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Mar 2, 2017

Uh oh!

sethmlarson commented Mar 2, 2017

Uh oh!

sethmlarson commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Mar 2, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sethmlarson commented Jan 18, 2017 •

edited

Loading

Uh oh!

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

sethmlarson commented Mar 2, 2017 •

edited

Loading

Uh oh!

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