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

Add trio.open_process, and deprecate Process(...) #1113

Merged
merged 3 commits into from
Jun 21, 2019

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Jun 17, 2019

Fixes gh-1109

@njsmith njsmith requested a review from oremanj June 17, 2019 08:15
@njsmith
Copy link
Member Author

njsmith commented Jun 17, 2019

Oh bleh, Windows wants to do async stuff in Process.__init__. So instead of shoving the whole thing off into a thread we have to shove just the Popen call into a thread and move the rest into open_process.

@njsmith
Copy link
Member Author

njsmith commented Jun 17, 2019

...which is actually pretty non-trivial, if we want to share the rest of the setup code between open_process and the legacy __init__ code during the deprecation period. I'm going to go to bed and see if I have any better ideas in the morning...

@oremanj
Copy link
Member

oremanj commented Jun 18, 2019

What's the additional async stuff that Windows needs in Process.init? Is it the creation of the named pipes?

@njsmith
Copy link
Member Author

njsmith commented Jun 18, 2019

yeah, and specifically registering them with IOCP, which requires finding the io manager's state

@oremanj
Copy link
Member

oremanj commented Jun 18, 2019

oh I see, not so much "wants to do blocking stuff" as "wants to do stuff in the Trio thread". Yeah that's a tough one :-( Will mull it over some.

As long as we have to support Process(...) for backcompat, it's not
worth the hassle:

  python-trio#1113 (comment)

So now this PR just changes the public interfaces, and then later
after we remove Process(...) we can do the actual fix without breaking
API.
@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

Merging #1113 into master will decrease coverage by 5.45%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
- Coverage   99.51%   94.05%   -5.46%     
==========================================
  Files         102      102              
  Lines       12519    12536      +17     
  Branches      953      953              
==========================================
- Hits        12458    11791     -667     
- Misses         40      696     +656     
- Partials       21       49      +28
Impacted Files Coverage Δ
trio/_subprocess.py 99.3% <100%> (-0.7%) ⬇️
trio/tests/test_subprocess.py 96.38% <100%> (-3.62%) ⬇️
trio/__init__.py 97.22% <100%> (ø) ⬆️
trio/_core/_windows_cffi.py 0% <0%> (-100%) ⬇️
trio/_wait_for_object.py 0% <0%> (-100%) ⬇️
trio/_subprocess_platform/windows.py 0% <0%> (-100%) ⬇️
trio/_windows_pipes.py 0% <0%> (-100%) ⬇️
trio/tests/test_wait_for_object.py 8.27% <0%> (-91.73%) ⬇️
trio/_core/_io_windows.py 0% <0%> (-91.29%) ⬇️
trio/_core/tests/test_windows.py 14.85% <0%> (-85.15%) ⬇️
... and 19 more

@codecov
Copy link

codecov bot commented Jun 20, 2019

Codecov Report

Merging #1113 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
+ Coverage   99.51%   99.51%   +<.01%     
==========================================
  Files         102      102              
  Lines       12519    12536      +17     
  Branches      953      953              
==========================================
+ Hits        12458    12475      +17     
  Misses         40       40              
  Partials       21       21
Impacted Files Coverage Δ
trio/_subprocess.py 100% <100%> (ø) ⬆️
trio/tests/test_subprocess.py 100% <100%> (ø) ⬆️
trio/__init__.py 97.22% <100%> (ø) ⬆️
trio/_highlevel_open_tcp_listeners.py 100% <0%> (ø) ⬆️

@njsmith
Copy link
Member Author

njsmith commented Jun 20, 2019

Eh, no brilliant ideas and it's not really worth fussing over for some code we're going to delete after the next release anyway.

For now I scaled back the PR to only making the interface changes, without the actual switch to using a thread, and then in a few months after Process(...) is gone we can fix the actual issue.

@oremanj
Copy link
Member

oremanj commented Jun 21, 2019

I'd love to be able to represent async with await ... as just async with ...; asyncssh has a decorator that does this. It would probably require us to stop pretending that the awaitable protocol doesn't exist, though, and is in any event a separate change. This looks good; thanks for making the change!

@oremanj oremanj merged commit 7eb1aca into python-trio:master Jun 21, 2019
@njsmith njsmith deleted the async-process-creation branch June 22, 2019 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Put process-creation into a thread
2 participants