search

python / asyncio

asyncio historical repository
https://docs.python.org/3/library/asyncio.html
1.04k stars 177 forks source link

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

Closed sethmlarson closed 7 years ago

sethmlarson commented 7 years ago

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.

sethmlarson commented 7 years ago

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

1st1 commented 7 years ago

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

sethmlarson commented 7 years ago

@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

1st1 commented 7 years ago

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.

sethmlarson commented 7 years ago

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

BotoX commented 7 years ago

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

1st1 commented 7 years ago

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.

sethmlarson commented 7 years ago

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.

sethmlarson commented 7 years ago

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

1st1 commented 7 years ago

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