Opened 8 years ago

Last modified 8 years ago

#6660 defect new

processExited example in listings/process/process.py erroneous

Reported by: Tom Sheffler Owned by:
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Branch: branches/robust-processterminated-example-6660
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

The process HOWTO is useful to get going with subprocess, but it's simplest example has "print" statements that can lead to exceptions when subprocess abnormally-end.

The documentation for processExisted says:

The status is passed in the form of a Failure instance, created with a .value that either holds a ProcessDone object if the process terminated normally (it died of natural causes instead of receiving a signal, and if the exit code was 0), or a ProcessTerminated object (with an .exitCode attribute) if something went wrong.

but in the same HOWTO article the intro code for processExited unconditionally assumes that the exitCode attribute is present:

    def processExited(self, reason):
        print "processExited, status %d" % (reason.value.exitCode,)

Using the above code as a building block of a new project leads to unexpected results if the 'reason' argument is indeed not a ProcessTerminated object.

Suggestion: Adjust the example to safely match the documentation.

Change History (7)

comment:1 Changed 8 years ago by Tom Sheffler

I have a new theory about this: the reason.value.exitCode can be "None" if the subprocess is killed by a signal and does not exit. In this case, the print statement causes and exception.

comment:2 Changed 8 years ago by Jean-Paul Calderone

It's confusing to say the process "does not exit". It might be true in the most literal sense - the process did not end because it called exit(3). However, clearly the process has ended and most people are probably comfortable saying it exited, too.

Please see the API documentation for ProcessTerminated. It explains the values that can be taken by the exitCode, signal, and status attributes.

My thoughts on this ticket are divided. As far as I can tell, the documentation is correct. It runs a child process which will exit normally (that is, not be terminated by a signal). At least, that is true unless some outside agent interferes and terminates the process using a signal.

comment:3 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/robust-processterminated-example-6660

(In [39796]) Branching to 'robust-processterminated-example-6660'

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

I did something in the branch. I would appreciate critical consideration of whether this is actually an *improvement* to the documentation or not.

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

Keywords: review added

comment:6 Changed 8 years ago by Tom Sheffler

Hi Exarkun - That is indeed unattractive, but is absolutely correct and very helpful!! so I"m torn too. (It would have been helpful to me.)

How about something like this?

    def processExited(self, reason):
        if (reason.status.exitCode == None):
            print "Process abnormally exited"
       else if (reason.status.exitCode == 0):
           print "Process exited normally"
      else:
          print "Process exited with exitcode:%d" % reason.status.exitCode

Let me explain how this got here. We built a compute farm of servers who inherited code that evolved from the outline in the docs. The docs printed the exit using "%d". On a large farm with many processes, some processes got killed by a supervisor program. Rather than reporting the signal, the "%d" printing exception was raised. It was very confusing.

For relative newcomers getting started with processes, it would be helpful to have a base outline that is correct and educational at the same time.

Just my few thoughts.

comment:7 Changed 8 years ago by lvh

Keywords: review removed

Hi,

I think JP's branch is a marked improvement (also it helps that the new version follows the style guide better).

My pet Twisted newbie thinks it's understandable (although he also understood the old version, and comprehension isn't the real issue here); that is: he understands the difference between the pieces of code. The only thing that they balked about is that the definition of _processEnd is before the two methods that use it, and they feel it's easier to read in the opposite order. I'm inclined to agree.

Also, I think format strings would be better here. Not just in general, but because you access a bunch of things on the same object, so: "{which}\n\tstatus: {e.status}, code: {e.exitCode}, signal: {e.signal}".format(which=which, e=exc), or somesuch.

I prefer JP's version to tom's.

I would recommend the minor cleanups, and think it's good to merge with or without them.

Note: See TracTickets for help on using tickets.