bpo-31884 subprocess set priority on windows #4150

JamesGKent · 2017-10-27T13:40:22Z

adds the required constants for setting process priority to the _winapi module
imports and makes available those constants in the subprocessing module

https://bugs.python.org/issue31884

adds the required constants that can be passed to creationflags for Popen to allow setting the process priority on windows.

Update subprocess.py

add constants for setting process priority

the-knights-who-say-ni · 2017-10-27T13:40:24Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

eryksun · 2017-10-27T17:20:35Z

You could also add the following creation flags:

CREATE_NO_WINDOW (allocate a console without a window)
DETACHED_PROCESS (do not inherit or allocate a console)
CREATE_DEFAULT_ERROR_MODE
CREATE_BREAKAWAY_FROM_JOB

The new constants should be documented in section "5.4.1. Constants", with a note about what was added in 3.7.

JamesGKent · 2017-10-28T12:45:20Z

Makes sense, however i thought it was already possible to allocate a console without a window via the startupinfo argument?

eryksun · 2017-10-28T19:53:35Z

When a console application initializes (i.e. the entry point of kernelbase.dll), if it doesn't inherit a console (i.e. a connection handle for an instance of conhost.exe), it allocates a new console and uses the process STARTUPINFO to configure it. This can be used to set the "WindowStation\Desktop"; screen buffer size and default fill attribute; and the window title, size, position, and whether or not to show (hide) the window. Currently we only support a small subset of STARTUPINFO, including the wShowWindow value.

Use the creation flag CREATE_NEW_CONSOLE to force the allocation of a new console instead of inheriting the parent's console. Note that the Secondary Logon service (i.e. CreateProcessWithLogonW and CreateProcessWithTokenW) always sets this creation flag when it calls CreateProcessAsUser. That's why runas.exe creates a new console. The CMD shell's start command also uses this flag, unless the /b option is used to run a console application in the 'background' (i.e. attached to the current console, but without waiting for it to exit).

Use DETACHED_PROCESS to prevent automatically attaching to a console. The standard handles will initially be NULL for a process created with this flag, unless they're redirected to another device. Some console applications assume valid standard handles, so generally you should redirect the standard handles to the NUL character device (i.e. subprocess.DEVNULL) when using this flag. A process without an automatically attached console can get one manually by calling AllocConsole or AttachConsole.

An issue with using DETACHED_PROCESS is that it doesn't prevent grandchild processes from automatically allocating a visible console. To avoid any visible console windows in the default case (i.e. no process in the tree is manually allocating a console), you can either use CREATE_NEW_CONSOLE with a STARTUPINFO that hides the console window or use the CREATE_NO_WINDOW flag, which makes the process allocate a windowless console. Either way, by default this console will be inherited by child processes. GetConsoleWindow returns NULL when attached to a console that has no window. In contrast, if it has a hidden window, you can pass the window handle to ShowWindow to reveal it. A console without a window or with a hidden window is otherwise functional as far as the API is concerned.

It's an error to specify both CREATE_NEW_CONSOLE and DETACHED_PROCESS, and if either is used CREATE_NO_WINDOW is ignored.

JamesGKent · 2017-10-30T09:59:41Z

thanks for the thorough explanation, i didn't realize the nuanced combinations of flags that are possible with this API and the effects this has for further child processes, i'll add to the pull request with these new flags and documentation.

added: CREATE_NO_WINDOW (allocate a console without a window) DETACHED_PROCESS (do not inherit or allocate a console) CREATE_DEFAULT_ERROR_MODE CREATE_BREAKAWAY_FROM_JOB

CREATE_NO_WINDOW (allocate a console without a window) DETACHED_PROCESS (do not inherit or allocate a console) CREATE_DEFAULT_ERROR_MODE CREATE_BREAKAWAY_FROM_JOB

JamesGKent · 2017-10-30T10:51:14Z

@eryksun can you clarify where the documentation needs to be ammended?

vstinner

You have to document these constants in the "Constants" section of Doc/library/subprocess.rst.

You have to add ".. versionadded:: 3.7" in the doc of added constants. Maybe add a subsection for Windows priority constants?

vstinner · 2017-11-06T23:47:55Z

test_subprocess fails on Windows (see AppVeyor):

  File "C:\projects\cpython\lib\test\test_subprocess.py", line 2900, in test__all__
    self.assertEqual(exported, possible_exports - intentionally_excluded)
AssertionError: Items in the first set but not the second:
'CREATE_BREAKAWAY_FROM_JOB'
'DETACHED_PROCESS'
'CREATE_DEFAULT_ERROR_MODE'
'CREATE_NO_WINDOW'

vstinner · 2017-11-07T14:49:00Z

Doc/library/subprocess.rst

+   :data:`CREATE_NO_WINDOW`
+   :data:`DETACHED_PROCESS`
+   :data:`CREATE_DEFAULT_ERROR_MODE`
+   :data:`CREATE_BREAKAWAY_FROM_JOB`


I'm not sure about this "versioadded" section. I suggest to remove it.

vstinner · 2017-11-07T14:49:24Z

Doc/library/subprocess.rst

+.. data:: ABOVE_NORMAL_PRIORITY_CLASS
+
+   A :class:`Popen` ``creationflags`` parameter to specify that a new process
+   will have an above average priority.


You have to add ".. versionadded:: 3.7" here: same for each added constant below.

vstinner · 2017-11-07T14:49:34Z

Doc/library/subprocess.rst

+.. data:: CREATE_BREAKAWAY_FROM_JOB
+
+  A :class:`Popen` ``creationflags`` parameter to specify that a new process
+  is not associated with the job.


Please add an empty line.

vstinner · 2017-11-07T14:49:52Z

Doc/library/subprocess.rst

@@ -516,8 +516,24 @@ functions.

   If given, *startupinfo* will be a :class:`STARTUPINFO` object, which is
   passed to the underlying ``CreateProcess`` function.
-   *creationflags*, if given, can be :data:`CREATE_NEW_CONSOLE` or
-   :data:`CREATE_NEW_PROCESS_GROUP`. (Windows only)
+   *creationflags*, if given, can be


This change seems unrelated and I preferred the previous formatting.

i'm not sure how this is unrelated? this shows where the added constants should be used?

Ah, when I wrote my comment, the doc was formatted differently.

Ok, now I suggest to format these constants as a list:

* a * b * c * ...

(correctly indented for the current section)

Try to build the documentation to see how it's rendered in HTML. On Unix, it's "make -C Doc html". I don't know how to build it on Windows :-(

unfortunately i don't currently have access to a unix box (windows 10 broke my dual boot) and currently on a work pc so don't even have git client, doing this through the web interface... so i can't build the docs to see how they look

creationflags, if given, can be

I checked in MSDN: in fact, you can specify multiple flags at once:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686331(v=vs.85).aspx

dwFlags: "A bitfield that determines whether certain STARTUPINFO members are used when the process creates a window. This member can be one or more of the following values."

By the way, please add a column after "can be" ("can be:"), and an empty line after.

I propose: "can be one or more of the following flags:".

bedevere-bot · 2017-11-07T14:49:59Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2017-11-07T15:45:32Z

Doc/library/subprocess.rst

+
+.. data:: ABOVE_NORMAL_PRIORITY_CLASS
+
+   .. versionadded:: 3.7


The versionadded markup is usually added after the text, and surrounded by an empty line (above and after).

vstinner · 2017-11-07T15:46:27Z

Doc/library/subprocess.rst

   The new process has a new console, instead of inheriting its parent's
   console (the default).

 .. data:: CREATE_NEW_PROCESS_GROUP
-
+
+   .. versionadded:: 3.7


This change seems wrong: this constant was already available on Python 3.6.

vstinner

Thanks. The overall change looks good to me, but I still have a few comments on the documentation which can be enhanced.

vstinner · 2017-11-08T11:39:10Z

Doc/library/subprocess.rst

@@ -843,6 +855,8 @@ The :mod:`subprocess` module exposes the following constants.
   The new process has a new console, instead of inheriting its parent's
   console (the default).

+   .. versionadded:: 3.7


That's wrong: the flag already existed in Python 3.6.

vstinner · 2017-11-08T11:39:26Z

Doc/library/subprocess.rst

@@ -851,6 +865,86 @@ The :mod:`subprocess` module exposes the following constants.

   This flag is ignored if :data:`CREATE_NEW_CONSOLE` is specified.

+   .. versionadded:: 3.7


That's wrong: the flag already existed in Python 3.6.

vstinner · 2017-11-08T13:11:35Z

Doc/library/subprocess.rst

+      * :data:`DETACHED_PROCESS`
+      * :data:`CREATE_DEFAULT_ERROR_MODE`
+      * :data:`CREATE_BREAKAWAY_FROM_JOB`
+


A single empty line is enough at the end of this list.

vstinner · 2017-11-08T13:12:30Z

Doc/library/subprocess.rst

@@ -843,6 +855,8 @@ The :mod:`subprocess` module exposes the following constants.
   The new process has a new console, instead of inheriting its parent's


The title of the current section is " Constants". I checked: all constants documented here as specific to Windows. I propose to rename the section to "Windows Constants". Can you please do that?

vstinner · 2017-11-08T13:15:14Z

Doc/library/subprocess.rst

@@ -516,8 +516,24 @@ functions.

   If given, *startupinfo* will be a :class:`STARTUPINFO` object, which is
   passed to the underlying ``CreateProcess`` function.
-   *creationflags*, if given, can be :data:`CREATE_NEW_CONSOLE` or
-   :data:`CREATE_NEW_PROCESS_GROUP`. (Windows only)
+   *creationflags*, if given, can be


creationflags, if given, can be

I checked in MSDN: in fact, you can specify multiple flags at once:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686331(v=vs.85).aspx

dwFlags: "A bitfield that determines whether certain STARTUPINFO members are used when the process creates a window. This member can be one or more of the following values."

By the way, please add a column after "can be" ("can be:"), and an empty line after.

I propose: "can be one or more of the following flags:".

vstinner · 2017-11-08T13:16:50Z

The doc job of Travis CI failed with:

[1] library/subprocess.rst:532: trailing whitespace
[1] library/subprocess.rst:915: trailing whitespace
[1] library/subprocess.rst:922: trailing whitespace

Please remove also these trailing spaces ;-)

vstinner

LGTM. Thank you for all updates! And I'm sorry for being so much pedantic about this PR ;-)

vstinner · 2017-11-08T13:59:26Z

@zooba: Nice, subprocess now will get the constants that you used in https://github.com/haypo/perf to set the priority ;-)

JamesGKent · 2017-11-08T14:11:30Z

i don't mind the being pedantic, its my first time contributing to a project this structured, but i wouldn't have it any other way. there is a reason that pythons docs are comprehensive and useful...
i only realized this was missing due to a question on stackoverflow and thinking "i'm sure you should be able to do that" defined the constants in the python script and it just worked, so figured they ought to be added to the module

vstinner · 2017-11-08T14:19:16Z

PR merged, thank again ;-) I changed the commit title.

JamesGKent added 4 commits October 27, 2017 14:33

add constants for setting process priority

22206b9

adds the required constants that can be passed to creationflags for Popen to allow setting the process priority on windows.

Update subprocess.py

788a086

Merge pull request #1 from JamesGKent/subprocess-priority-windows-1

f47ce08

Update subprocess.py

Merge pull request #2 from JamesGKent/subprocess-priority-windows

a7e78e4

add constants for setting process priority

JamesGKent requested a review from gpshead as a code owner October 27, 2017 13:40

bedevere-bot added the awaiting review label Oct 27, 2017

the-knights-who-say-ni added the CLA not signed label Oct 27, 2017

Create 2017-10-27.bpo-31884.bjhre9.rst

0b3e805

JamesGKent changed the title ~~subprocess set priority on windows~~ bpo-31884 subprocess set priority on windows Oct 27, 2017

added constants

bdaf817

added: CREATE_NO_WINDOW (allocate a console without a window) DETACHED_PROCESS (do not inherit or allocate a console) CREATE_DEFAULT_ERROR_MODE CREATE_BREAKAWAY_FROM_JOB

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 30, 2017

added constants

f972658

CREATE_NO_WINDOW (allocate a console without a window) DETACHED_PROCESS (do not inherit or allocate a console) CREATE_DEFAULT_ERROR_MODE CREATE_BREAKAWAY_FROM_JOB

vstinner reviewed Nov 6, 2017

View reviewed changes

JamesGKent added 3 commits November 7, 2017 12:10

fixed missing import

55ace15

Update subprocess.rst

13f0893

Update subprocess.rst

382f890

vstinner requested changes Nov 7, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 7, 2017

JamesGKent added 2 commits November 7, 2017 15:37

Update subprocess.rst

197861f

Update subprocess.rst

2ccf787

vstinner reviewed Nov 7, 2017

View reviewed changes

JamesGKent added 2 commits November 7, 2017 15:49

Update subprocess.rst

680efd7

Update subprocess.rst

5114f8c

vstinner reviewed Nov 8, 2017

View reviewed changes

JamesGKent added 3 commits November 8, 2017 13:24

Update subprocess.rst

8d3fac6

Update subprocess.rst

264fb12

fixed underline too short

0765c90

vstinner approved these changes Nov 8, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Nov 8, 2017

vstinner merged commit b5d9e08 into python:master Nov 8, 2017

bedevere-bot removed the awaiting merge label Nov 8, 2017

ethanhs mentioned this pull request Dec 3, 2018

Create new, hidden window when starting daemon on Windows python/mypy#5994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31884 subprocess set priority on windows #4150

bpo-31884 subprocess set priority on windows #4150

JamesGKent commented Oct 27, 2017 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Oct 27, 2017

eryksun commented Oct 27, 2017

JamesGKent commented Oct 28, 2017

eryksun commented Oct 28, 2017

JamesGKent commented Oct 30, 2017

JamesGKent commented Oct 30, 2017

vstinner left a comment

vstinner commented Nov 6, 2017

vstinner Nov 7, 2017

vstinner Nov 7, 2017

vstinner Nov 7, 2017

vstinner Nov 7, 2017

JamesGKent Nov 7, 2017

vstinner Nov 7, 2017

JamesGKent Nov 7, 2017

vstinner Nov 8, 2017

bedevere-bot commented Nov 7, 2017

vstinner Nov 7, 2017

vstinner Nov 7, 2017

vstinner left a comment

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner commented Nov 8, 2017

vstinner left a comment

vstinner commented Nov 8, 2017

JamesGKent commented Nov 8, 2017

vstinner commented Nov 8, 2017

		@@ -851,6 +865,86 @@ The :mod:`subprocess` module exposes the following constants.

		This flag is ignored if :data:`CREATE_NEW_CONSOLE` is specified.

		.. versionadded:: 3.7

		@@ -843,6 +855,8 @@ The :mod:`subprocess` module exposes the following constants.
		The new process has a new console, instead of inheriting its parent's

bpo-31884 subprocess set priority on windows #4150

bpo-31884 subprocess set priority on windows #4150

Conversation

JamesGKent commented Oct 27, 2017 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Oct 27, 2017

eryksun commented Oct 27, 2017

JamesGKent commented Oct 28, 2017

eryksun commented Oct 28, 2017

JamesGKent commented Oct 30, 2017

JamesGKent commented Oct 30, 2017

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 8, 2017

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Nov 8, 2017

JamesGKent commented Nov 8, 2017

vstinner commented Nov 8, 2017

JamesGKent commented Oct 27, 2017 •

edited by bedevere-bot

Loading