Delay closing SubprocessStreamProtocol's Transport until all pipes are closed. by sethmlarson · Pull Request #485 · python/asyncio · GitHub

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.

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.

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! :) )

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

@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

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.

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. :)

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

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

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

...removal from fd list does not occur.

What does -1 mean here?

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.

Should I use a better sentinel object than -1?

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

Sure thing!

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.

Sure, just dedent that line then?

yep.

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

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

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.

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.

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

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

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

Add simple test for patch

12428a2

Remove comment about /dev/urandom

33cd1c5

1st1 reviewed Mar 2, 2017

View reviewed changes

Allow _maybe_close_transport to close even if...

16a04d2

...removal from fd list does not occur.

1st1 reviewed Mar 2, 2017

View reviewed changes

Add flag for process_exited, separate this from...

dc23e3f

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

sethmlarson mentioned this pull request Mar 2, 2017

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

Merged

1st1 closed this Mar 2, 2017

sethmlarson deleted the patch-1 branch March 2, 2017 22:25

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

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

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

Delay closing SubprocessStreamProtocol's Transport until all pipes are closed. by sethmlarson · Pull Request #485 · python/asyncio · GitHub

Uh oh!

Uh oh!

Delay closing SubprocessStreamProtocol's Transport until all pipes are closed. - #485

Delay closing SubprocessStreamProtocol's Transport until all pipes are closed.#485sethmlarson wants to merge 5 commits intopython:masterpython/asyncio:masterfrom sethmlarson:patch-1Copy head branch name to clipboard

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

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Delay closing SubprocessStreamProtocol's Transport until all pipes are closed.#485
sethmlarson wants to merge 5 commits into
python:masterfrom
sethmlarson:patch-1

sethmlarson commented Jan 18, 2017 •
edited
Loading

sethmlarson commented Mar 2, 2017 •
edited
Loading