Opened 2 years ago

Last modified 2 years ago

#5726 defect new

spawnProcess opens an unwanted console

Reported by: alecu Owned by: alecu
Priority: normal Milestone:
Component: core Keywords: windows console
Cc: allister.macleod@… Branch: branches/windows-spawnprocess-nowindow-5726
(diff, github, buildbot, log)
Author: amacleod Launchpad Bug:

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 alecu 2 years ago.
Use the CREATE_NO_WINDOW flag to call CreateProcess
dont-open-console-window-v2.diff (3.6 KB) - added by alecu 2 years ago.
Fixes issues requested on the first review

Download all attachments as: .zip

Change History (14)

Changed 2 years ago by alecu

Use the CREATE_NO_WINDOW flag to call CreateProcess

comment:2 Changed 2 years ago by alecu

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 2 years ago by thijs

  • Keywords review added

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

comment:4 Changed 2 years ago by exarkun

  • Keywords changed from windows, console, review to windows console review
  • Summary changed from [windows] spawnProcess opens an unwanted console to spawnProcess opens an unwanted console

comment:5 Changed 2 years ago by amacleod

  • Cc allister.macleod@… added

comment:6 Changed 2 years ago by amacleod

  • Owner set to amacleod

comment:7 Changed 2 years ago by amacleod

  • Keywords review removed
  • Owner changed from amacleod to alecu

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 2 years ago by amacleod

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 2 years ago by alecu

Fixes issues requested on the first review

comment:9 Changed 2 years ago by alecu

  • Keywords review added
  • Owner alecu 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 2 years ago by amacleod

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

comment:11 Changed 2 years ago by amacleod

  • Author set to amacleod
  • Branch set to branches/windows-spawnprocess-nowindow-5726

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

comment:12 Changed 2 years ago by amacleod

  • Keywords review removed
  • Owner set to alecu

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)

Note: See TracTickets for help on using tickets.