Opened 5 years ago

Last modified 9 days ago

#5726 defect new

spawnProcess opens an unwanted console

Reported by: Alejandro J. Cura Owned by: Craig Rodrigues
Priority: normal Milestone:
Component: core Keywords:
Cc: Allister MacLeod, Jean-Paul Calderone Branch: 5726-rodrigc-windows-spawnprocess-nowindow
branch-diff, diff-cov, branch-cov, buildbot
Author: amacleod

Description

When spawning a process under Windows to capture its standard output, if the controlling process is not a console application a new console is wrongly opened.

Here's a sample program that shows the bug:

# should_not_open_console.pyw

from twisted.internet import reactor, utils

def write_result(result):
    open("output.log", "w").write(repr(result))
    reactor.stop()

PING_EXE = r"c:\windows\system32\ping.exe"
d = utils.getProcessOutput(PING_EXE, ["slashdot.org"])
d.addCallbacks(write_result)
reactor.run()

Make sure to run it with the pythonw executable:

c:\python27\pythonw.exe should_not_open_console.pyw

When running the above command, the program starts in the background, and the wrong behavior shows up: a new console is opened, with the path to ping.exe as the title, and it remains blank while ping runs.

The expected behavior is: no new console is opened.

Attachments (2)

dont-open-console-window.diff (2.4 KB) - added by Alejandro J. Cura 5 years ago.
Use the CREATE_NO_WINDOW flag to call CreateProcess
dont-open-console-window-v2.diff (3.6 KB) - added by Alejandro J. Cura 5 years ago.
Fixes issues requested on the first review

Download all attachments as: .zip

Change History (20)

Changed 5 years ago by Alejandro J. Cura

Use the CREATE_NO_WINDOW flag to call CreateProcess

comment:2 Changed 5 years ago by Alejandro J. Cura

Following one of the solutions proposed in the threads above, I've submitted a fix that uses the CREATE_NO_WINDOW flag to call CreateProcess, as described in Process Creation Flags.

comment:3 Changed 5 years ago by Thijs Triemstra

Keywords: review added

Putting it up for review as described on ReviewProcess#Authors:Howtogetyourchangereviewed.

comment:4 Changed 5 years ago by Jean-Paul Calderone

Keywords: windows, console, reviewwindows console review
Summary: [windows] spawnProcess opens an unwanted consolespawnProcess opens an unwanted console

comment:5 Changed 5 years ago by Allister MacLeod

Cc: Allister MacLeod added

comment:6 Changed 5 years ago by Allister MacLeod

Owner: set to Allister MacLeod

comment:7 Changed 5 years ago by Allister MacLeod

Keywords: review removed
Owner: changed from Allister MacLeod to Alejandro J. Cura

I reviewed the patch and it mostly looks good. Things still to do:

  1. Needs a news fragment.
  2. In the test (test_process.py:2304) it might be worth explaining that index 5 is the positional argument for flags.
  3. It would be nice to capture the steps to reproduce this behavior in Twisted source somewhere. I'm guessing that making tests that require user interaction (a la https://launchpad.net/game) would be inappropriate to the Twisted suite, and I'm not even sure if there is a way to reliably force repro from trial. Anyway, if you can think of a way to explain and capture the issue in source, that would be cool. If not, I think that's ok too.

I verified that the test passes on Windows 7 64-bit running Python 2.7.3 32-bit with Twisted 12.1.0 source patched with your diff. I also verified that the test fails if I revert _dumbwin32proc.py

I also observed that, pre-patch, the extraneous window shows up if I run the .pyw in the ticket description and post-patch the program runs without any extra windows appearing.

comment:8 Changed 5 years ago by Allister MacLeod

Oh, one more thing. I heard some murmuring in #twisted-dev that sibpath ought to be deprecated, and you should use FilePath(x).sibling("y") instead.

Changed 5 years ago by Alejandro J. Cura

Fixes issues requested on the first review

comment:9 Changed 5 years ago by Alejandro J. Cura

Keywords: review added
Owner: Alejandro J. Cura deleted

I've attached a new patch with the fixes requested on the review:

  • added a News fragment
  • makes more explicit the parameter being checked in the tests
  • improves the docstring for the test, and includes this issue number
  • uses FilePath(x).sibling instead of sibpath

comment:10 Changed 5 years ago by Allister MacLeod

Fixes look good. I will see about getting these changes committed.

comment:11 Changed 5 years ago by Allister MacLeod

Author: amacleod
Branch: branches/windows-spawnprocess-nowindow-5726

(In [35150]) Branching to 'windows-spawnprocess-nowindow-5726'

comment:12 Changed 5 years ago by Allister MacLeod

Keywords: review removed
Owner: set to Alejandro J. Cura

As exarkun was coaching me through pre-commit steps he pointed out some issues with the patch:

  1. The new test is in twisted/test/, to which we ought to avoid adding new things. It might be more appropriate to make a new TestCase dedicated to Win32 process stuff. JP suggested a new module in twisted/internet/test/.
  2. There are naming convention mistakes in the test. The one I can spot from a brief scan is that Win32ProcessFlagsTest's sibling classes seem to end in "TestCase".
  3. We ought not to be using @inlineCallbacks on test methods.

One question I think worth considering is how much merit there is in a unit test for behavior that's easy for humans to observe but hard for trial to observe. I think the test you wrote is worthy, since it at least makes sure that the right flags are being passed to and fro. I would suggest altering its docstring a little, though, to clearly state that it's just testing that flags get set properly and the function call doesn't throw an exception.

So, short of coming up with a magical Python library that can detect the presence and absence of windows, I think moving the current test code to its own module and fixing up any naming issues should be sufficient. (http://twistedmatrix.com/documents/current/core/development/naming.html)

comment:13 Changed 3 months ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

comment:14 Changed 3 months ago by Jean-Paul Calderone

I wonder where the branch for this issue went.

comment:15 Changed 3 months ago by Jean-Paul Calderone

Ah, it is on Github after all.

comment:16 Changed 3 months ago by Jean-Paul Calderone

Branch: branches/windows-spawnprocess-nowindow-5726windows-spawnprocess-nowindow-5726

comment:17 Changed 4 weeks ago by Craig Rodrigues

Branch: windows-spawnprocess-nowindow-57265726-rodrigc-windows-spawnprocess-nowindow
Keywords: review added
Owner: Alejandro J. Cura deleted

Reviving this patch and submitting to GitHub: https://github.com/twisted/twisted/pull/697

comment:18 Changed 9 days ago by Glyph

Keywords: review windows console removed
Owner: set to Craig Rodrigues

Thanks for the update, Craig! Reviewed and approved on Github:

https://github.com/twisted/twisted/pull/697#pullrequestreview-21909861

Note: See TracTickets for help on using tickets.