Why is this subprocess.check_output line crashing my script?

Question

I have a script that works fine when in a .pyw, but when converted to an .exe, (EDIT: actually, when I use pyinstaller with the arguments -w or --windowed or --noconsole it doesn't work, but without them it works) I've found that this one single line seems to crash the program:

firstplan = subprocess.check_output(["powercfg", "-list"], shell=True ).split('\n')[3]

Does anybody have any idea why? If I comment it out the program doesn't crash. I have two other very similar lines.

EDIT:

Maybe it'd be a good idea to put the script here...

from __future__ import print_function
import os
# os.system('cls')
import psutil
import subprocess

loop = 1

while loop == (1):

    CPUload = (psutil.cpu_percent(interval=4))      # CPU load
    RAMload = (psutil.virtual_memory().percent)     # RAM load

    # os.system('cls')

    print("CPU Load: ", end="")         # Display CPU Load:
    print(CPUload, "%")                 # Display CPUload
    print("RAM Load: ", end="")         # Display CPU Load:
    print(str(RAMload) + " %")          # Display RAMload

    firstplan = subprocess.check_output(["powercfg", "-list"], shell=True ).split('\n')[3]      # Selects a line
    secondplan = subprocess.check_output(["powercfg", "-list"], shell=True ).split('\n')[4]
    thirdplan = subprocess.check_output(["powercfg", "-list"], shell=True ).split('\n')[5]

    firstplanID = ((firstplan.split(": "))[1].split("  (")[0])      # Extract XplanID from Xplan
    secondplanID = ((secondplan.split(": "))[1].split("  (")[0])
    thirdplanID = ((thirdplan.split(": "))[1].split("  (")[0])

    activeplan = subprocess.check_output(["powercfg", "/getactivescheme"])      # Find the currently active plan
    activeplanNAME = ((activeplan.split("("))[1].split(")")[0])     # Extract activeplanNAME from activeplan

    firstplanNAME = ((firstplan.split("("))[1].split(")")[0])       # Extract XplanNAME from Xplan
    secondplanNAME = ((secondplan.split("("))[1].split(")")[0])
    thirdplanNAME = ((thirdplan.split("("))[1].split(")")[0])


    if "High performance" in firstplanNAME:         # Identify which plan is High performance
        HighPerformance = firstplanNAME
        HighPerformanceID = firstplanID

    if "High performance" in secondplanNAME:
        HighPerformance = secondplanNAME
        HighPerformanceID = secondplanID

    if "High performance" in thirdplanNAME:
        HighPerformance = thirdplanNAME
        HighPerformanceID = thirdplanID

    if "Power saver" in firstplanNAME:              # Identify which plan is Power saver
        PowerSaver = firstplanNAME
        PowerSaverID = firstplanID

    if "Power saver" in secondplanNAME:
        PowerSaver = secondplanNAME
        PowerSaverID = secondplanID

    if "Power saver" in thirdplanNAME:
        PowerSaver = thirdplanNAME  
        PowerSaverID = thirdplanID


    if activeplanNAME == PowerSaver:            # Checks current plan name
        print("Active plan: Power saver")
    else:
        if activeplanNAME == HighPerformance:
            print("Active plan: High Performance")
        else: 
            subprocess.check_output(["powercfg", "/s", HighPerformanceID])          


    if CPUload < 44:    
        if RAMload > 90:
            if activeplanNAME == PowerSaver:
                subprocess.check_output(["powercfg", "/s", HighPerformanceID])
                print("Switching to High Performance by RAM load...")       

    if CPUload < 44:    
        if RAMload < 90:
            if activeplanNAME == HighPerformance:
                subprocess.check_output(["powercfg", "/s", PowerSaverID])
                print("Switching to Power saver...")                    

    if CPUload > 55:
        if activeplanNAME == PowerSaver:
            subprocess.check_output(["powercfg", "/s", HighPerformanceID])
            print("Switching to High Performance...")

The troubled lines are lines 21-23.

For further info, scroll down to comments and answers.

Passing the commands as a list with `shell=True` apparently happens to work on some platforms, but is really not the right thing. Take out `shell=True`. (I don't think this will solve your problem, though.) — tripleee, Aug 06 '18 at 05:24
The indentation in the script you shared is clearly wrong (some or all of the lines after the `while` loop near the beginning should be indented). Please take care to post valid code; indentation is significant with Python in particular. See also [Markdown help](/editing-help). — tripleee, Aug 06 '18 at 05:25
@tripleee What do you mean not really the right thing? Also yeah I took out the `shell=True` before and it still didn't work. Also I will fix the indentation, it is indented in my script but I forgot to indent on the site. — jangles, Aug 06 '18 at 05:27
The `subprocess` documentation has the details. If you use `shell=True` you should pass in a string for the shell to parse; with `shell=False` (or no `shell=True`) you need to pass in a list of parsed tokens. — tripleee, Aug 06 '18 at 05:28
@tripleee - I'm sorry I'm new to Python, should I just take out the `shell=True` entirely to make it `firstplan = subprocess.check_output(["powercfg", "-list"]).split('\n')[3]`? Or should I change it to false? — jangles, Aug 06 '18 at 05:31
Explicit is better than implicit so change to `shell=False` although technically taking it out will do the same thing. — tripleee, Aug 06 '18 at 05:41
Calling a shell three times to get three lines of output from the same command is also pretty wasteful. Just do `pcfg = subprocess.check_output(['powercfg', '-list']).split('\n')` and then `first = pcfg[3]`, `second = pcfg[4]` etc. — tripleee, Aug 06 '18 at 05:43
Does the command really accept `-list` as an option and even if it does, wouldn't it be better to use `/list` for consistency? (Assuming the command is consistent in the first place. This is apparently Windows, after all...) — tripleee, Aug 06 '18 at 05:45
@tripleee - You're right about the consistency. I must have missed that! Thank you so much for helping me with this. I used the pcfg value like this: `pcfg = subprocess.check_output(['powercfg', '/list']).split('\n') and (then (firstplan = pcfg[3], secondplan = pcfg[4], thirdplan = pcfg[5]))` and it tells me after `firstplan` that it is invalid syntax. — jangles, Aug 06 '18 at 05:56
Hold on I realized now that was dumb. I now have `pcfg = subprocess.check_output(['powercfg', '/list']).split('\n')` `firstplan = pcfg[3], secondplan = pcfg[4], thirdplan = pcfg[5]` and it says that there are too many values to unpack. I think it has something to do with how doing `print(pcfg)` gives the values is separate strings instead of separate lines like how it was before. — jangles, Aug 06 '18 at 06:03
You need the assignments on separate lines. Alternatively, Python allows you to put semicolons between statements, but this is generally avoided. — tripleee, Aug 06 '18 at 06:37
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/177465/discussion-between-jangles-and-tripleee). — jangles, Aug 06 '18 at 07:06
Just a stylistic point to make your code less cluttered, instead of 3x3 variables `firstplan, firstplanID, firstplanNAME...` why not just have arrays `plan[0:2], plan_id[0:2], plan_name[0:2]` or even declare a class whose __init__() takes the raw output from `subprocess.check_output` and extracts `id` and `name` string fields. (CodeReview.SE site is better for soliciting general comments like that) — smci, Aug 18 '18 at 03:46

tripleee · Accepted Answer · 2018-08-06T07:36:27.983

I'm not sure if this will solve your problem, but here is a refactoring which addresses the problems which were pointed out in comments, as well as some other issues with your code.

Don't use a loop variable. (It was unused anyway.)
Don't run the same subprocess three times.
Avoid the gratuitous shell=True
Prefer /list over -list for consistency and possibly correctness.

I have removed your comments and inlined comments of mine explaining what exactly was changed.

# Only necessary in Python 2, you really should be using Python 3 now
#from __future__ import print_function
# Only used in os.system('cls') which was commented out (for good reasons I presume)
#import os
import psutil
import subprocess
from time import sleep # see below

# Simply loop forever; break when done
while True:
    # Remove gratuitous parentheses
    CPUload = psutil.cpu_percent(interval=4)
    RAMload = psutil.virtual_memory().percent

    # Use .format() to inline a string
    print("CPU Load: {}%".format(CPUload)))
    print("RAM Load: {}%".format(RAMload))

    # Only run subprocess once; use /list pro -list; don't use shell=True
    pcfg = subprocess.check_output(["powercfg", "/list"], shell=False).split('\n')
    # Additional refactoring below ########
    firstplan = pcfg[3]
    secondplan = pcfg[4]
    thirdplan = pcfg[5]

    # Get rid of wacky parentheses    
    firstplanID = firstplan.split(": ")[1].split("  (")[0]
    secondplanID = secondplan.split(": ")[1].split("  (")[0]
    thirdplanID = thirdplan.split(": ")[1].split("  (")[0]

    activeplan = subprocess.check_output(["powercfg", "/getactivescheme"])
    # Get rid of wacky parentheses
    activeplanNAME = activeplan.split("(")[1].split(")")[0]    
    firstplanNAME = firstplan.split("(")[1].split(")")[0]
    secondplanNAME = secondplan.split("(")[1].split(")")[0]
    thirdplanNAME = thirdplan.split("(")[1].split(")")[0]

    if "High performance" in firstplanNAME:
        HighPerformance = firstplanNAME
        HighPerformanceID = firstplanID

    if "High performance" in secondplanNAME:
        HighPerformance = secondplanNAME
        HighPerformanceID = secondplanID

    if "High performance" in thirdplanNAME:
        HighPerformance = thirdplanNAME
        HighPerformanceID = thirdplanID

    if "Power saver" in firstplanNAME:
        PowerSaver = firstplanNAME
        PowerSaverID = firstplanID

    if "Power saver" in secondplanNAME:
        PowerSaver = secondplanNAME
        PowerSaverID = secondplanID

    if "Power saver" in thirdplanNAME:
        PowerSaver = thirdplanNAME  
        PowerSaverID = thirdplanID

    # Additional refactoring ends    

    if activeplanNAME == PowerSaver:
        print("Active plan: Power saver")
    # prefer if / elif / else over nested if
    elif activeplanNAME == HighPerformance:
        print("Active plan: High Performance")
    else:
        # What's this supposed to do? You are capturing, then discarding the output.
        # Perhaps you are looking for subprocess.check_call()?
        subprocess.check_output(["powercfg", "/s", HighPerformanceID])          

    if CPUload < 44:    
        # Combine conditions rather than nesting conditionals
        if RAMload > 90 and activeplanNAME == PowerSaver:
            # subprocess.check_call() here too?
            subprocess.check_output(["powercfg", "/s", HighPerformanceID])
            print("Switching to High Performance by RAM load...")       

        # Don't check if CPUload < 44: again
        # Instead, just stay within this indented block
        # Combine conditions
        elif RAMload < 90 and activeplanNAME == HighPerformance:
            # subprocess.check_call() here too?
            subprocess.check_output(["powercfg", "/s", PowerSaverID])
            print("Switching to Power saver...")                    

        # What if RAMload == 90?

    # Combine conditions
    if CPUload > 55 and activeplanNAME == PowerSaver:
        # subprocess.check_call() here too?
        subprocess.check_output(["powercfg", "/s", HighPerformanceID])
        print("Switching to High Performance...")

    # Maybe sleep between iterations?
    #sleep(1)

The script currently runs a rather tight loop, you might want to uncomment the last line.

There is still a lot of repetitive code. You might want to consider further refactoring to collect the three plans into an array where each object is a dict with member names identifying different properties you have extracted.

    # Additional refactoring below ########
    activeplan = subprocess.check_output(["powercfg", "/getactivescheme"])
    activeplanNAME = activeplan.split("(")[1].split(")")[0]    

    plan = []
    for idx in range(3):
        raw = pcfg[3+idx]
        thisplan = {'raw': raw}
        thisplan['id'] = raw.split(": ")[1].split("  (")[0]
        thisplan['name'] = raw.split("(")[1].split(")")[0]

        if "High performance" in thisplan['name']:
            HighPerformance = thisplan['name']
            HighPerformanceID = thisplan['id']    

        if "Power saver" in thisplan['name']:
            PowerSaver = thisplan['name']
            PowerSaverID = thisplan['id']

        plan[idx] = thisplan

Now you could actually change the HighPerformance and PowerSaver variables to just remember the idx and then if you want to, you can pull out the name from the list of dicts with plan[PowerSaverIdx]['name'] and ID with plan[PowerSaverIdx]['id'].

So I found this: https://stackoverflow.com/a/51706087/8142044 and that fixed the script crashing when in a `--noconsole` mode and after I took out `stdout-subprocess.PIPE`. I put this for every `subprocess.check_output`. However, the program will flash a command prompt every couple seconds which defeats the purpose of a background running process. — jangles, Aug 06 '18 at 17:47
Sorry, can't help with that. My solution was to abandon Windows entirely and it was one of the best decisions I've made. — tripleee, Aug 06 '18 at 18:12
Okay thank you for your help though! I will start a new question. Also what OS do you use? — jangles, Aug 06 '18 at 18:18
Blanket recommendations are problematic but for what I do, I prefer Debian Linux. — tripleee, Aug 07 '18 at 02:25

Why is this subprocess.check_output line crashing my script?

1 Answers1