Merge twisted.internet.process.PTYProcess and Process

[itamarst]

twisted.internet.process.PTYProcess and Process share a lot of the same code; they should be merged as much as possible. This should ensure bug fixes like #2305 don't need to be duplicated.

#1939 can function as a more general "make process API better" issue, this is just a non-user-visible refactor of internals.

[therve]

OK now #2419 is in, we should move os.kill handling out of here. We still agree on doing this one before #2420 ?

[Jean-Paul Calderone]

That should be the simpler order, yea.

[therve]

I merged the branch and removed os.kill error handling (will go in #2420).

The behavior of PTYProcess.doRead changed, and I had to fix one stdio test because of it. Otherwise everything seems fine.

Itamar, I let you do put this into review when OK ?

[itamarst]

This needs some new tests, especially for PTY support. See the changes for applicable things; the changes probably shouldn't be in the code.

[therve]

OK, I would have love that #2598 be available, but it may take ages before being in trunk, so let's wake up this ticket.

[itamarst]

Did the tests for the changes get added? Also the list of changed functionality in this branch should be added to the ticket so reviewers can look them over.

[therve]

Here are the changes you noted:

PTYProcess:

• No longer catch OSError and check for ECHILD in reapProcess, and raise if not (bad!)
• Now raises ProcessExitedAlready in signalProcess
• Used to write errors in child process to fd 1, now writes to fd 2
• On errors in child process, fds 1-3 are now closed
• setregid and setreuid are now called, unlike before. Is this OK?

Both:

• setuid/gid(0) now done after os.pipe()/openpty() are called. Is this OK?
• handle case where process exited but twisted hasn't yet noticed

I think the changes of setreuid/gid and setuid/gid aren't tested (to check). The rest is.

[Jean-Paul Calderone]

Pre-existing defects near changed code:

• process.py
  • registerReapProcessHandler has no docstring or unit tests. The exception it raises should have a message of some sort, too.
  • unregisterReapProcessHandler has no docstring or unit tests. The exception it raises should have a message of some sort, too.
  • detectLinuxBrokenPipeBehavior has no docstring
  • no test coverage for the setuid-related code

These can probably have tickets filed for them and be fixed in later branches.

Some hopefully trivial things:

• windows has a couple failures now, since it doesn't have the fcntl module
• _fork's docstring should document the type and meaning of each of its arguments
• Process.__init__ and PTYProcess.__init__ still have a lot of overlap and slightly differing behavior. eg, PTYProcess calls FileDescriptor.__init__, Process doesn't. A bit more refactoring in this area might not be amiss.
• The change in PTYProcess.doRead to call outReceived instead of childDataReceived doesn't seem correct to me. What's up there?

[therve]

Replying to exarkun:

Pre-existing defects near changed code:

• process.py
  • registerReapProcessHandler has no docstring or unit tests. The exception it raises should have a message of some sort, too.

I've added a docstring, and changed the exception.

• unregisterReapProcessHandler has no docstring or unit tests. The exception it raises should have a message of some sort, too.

I've added a docstring, and changed the exception.

• detectLinuxBrokenPipeBehavior has no docstring

Done.

• no test coverage for the setuid-related code

I've added something, better than nothing.

These can probably have tickets filed for them and be fixed in later branches.

Some hopefully trivial things:

• windows has a couple failures now, since it doesn't have the fcntl module

I'm trying to correct that. That shouldn't be too hard.

• _fork's docstring should document the type and meaning of each of its arguments

Done.

• Process.__init__ and PTYProcess.__init__ still have a lot of overlap and slightly differing behavior. eg, PTYProcess calls FileDescriptor.__init__, Process doesn't. A bit more refactoring in this area might not be amiss.

Process doesn't call FileDescriptor.__init__ because it's not a Filedescriptor :). I add a quick look to other code, and didn't find a clean way to factor more. I can look further though.

• The change in PTYProcess.doRead to call outReceived instead of childDataReceived doesn't seem correct to me. What's up there?

My bad, I just took a shortcut. I revert that.

[therve]

win32 buildbot is green, so I put it back in review.

[Jean-Paul Calderone]

The code is in /branches/merge-process-and-ptyprocess-2341-2 btw

[Jean-Paul Calderone]

• process.py
  • registerReapProcessHandler
    • auxPid should be auxPID since PID is an acronym
    • It doesn't seem to be documented anywhere in Python that os.waitpid returns (0, 0) when there are children but none have exited. Maybe it'd be good to mention that in a comment near the call so it's more obvious why the pid is checked the way it is.
  • _BaseProcess
    • in _fork, command would probably be better named executable
    • in reapProcess, it'd be nice to preserve the ECHILD check which PTYProcess.reapProcess performed. We shouldn't log tracebacks which we can easily determine are definitely not errors.
• test_process.py
  • TrivialProcessProtocol probably shouldn't use class attributes for lists it mutates

Still need to review the rest of test_process.py, but this seems like a good pausing place and I'm out of time for now. Tackle the above for now if you want, and I'll try to get to the rest of this review soon.

Nice work on this, btw. It's pretty hair and almost all of the implementation refactoring looks good.

[itamarst]

JP, thanks for looking at this. As part of review, can you look at the changes noted in a comment 9 above? Thanks.

[therve]

(In [20921]) Apply review comments

Refs #2341

[therve]

(In [20922]) Rename command to executable.

Refs #2341

[therve]

OK I think I tackled the points. I renamed command to executable everywhere to be consistent. Thanks for the review.

[Jean-Paul Calderone]

Okay, in test_process.py:

• test_process gains a new assertion about signalProcess raising an exception, but only on platforms other than windows? I guess it seems like this functionality is broken on windows, but maybe that's a bug. not sure what the best approach to this is. I guess a ticket should be filed, at least. if the test stays as it is now for the merge (which might be okay), it should probably get a comment about how it should be passing on windows too.
• Maybe the /bin/true finding code could be factored out so it's not repeated 3 times
• MockOS looks like it could be something really nice :) It'd be nice if all its methods had docstrings, and some whitespace would help too.
• The MockProcessTestCase test method docstring could use some help too. They're not too specific about what is actually being tested.
• The mockSetUid test could cover the PTYProcess bug which was fixed in this refactoring if MockOS remembered what the active uid/euid values were (so the test could check that they got put back) and if fork could be customized to pretend to be either the parent or the child. this might make sense as another ticket though.

[Jean-Paul Calderone]

Replying to therve:

Here are the changes you noted:

PTYProcess:

• No longer catch OSError and check for ECHILD in reapProcess, and raise if not (bad!)

This is fixed in the branch now.

• Now raises ProcessExitedAlready in signalProcess

This is good. (If only win32proc did the same. ;)

• Used to write errors in child process to fd 1, now writes to fd 2

This is an acceptable change. 1 and 2 are the same. Ultimately we should report errors in a way which is better than this one, but I don't see this change as harmful for now.

• On errors in child process, fds 1-3 are now closed

This is fine.

• setregid and setreuid are now called, unlike before. Is this OK?

This is, in fact, amazingly critical, and the failure to do so before was an unbelievably stupendous bug. The main shortcoming in the branch now is that there is no test coverage for this new correct behavior.

Both:

• setuid/gid(0) now done after os.pipe()/openpty() are called. Is this OK?

Yep, totally fine.

• handle case where process exited but twisted hasn't yet noticed

I'm not sure what this refers to.

I think the changes of setreuid/gid and setuid/gid aren't tested (to check). The rest is.

That basically agrees with the conclusion I reached, I think.

[itamarst]

"Now raises ProcessExitedAlready in signalProcess" is, I think, now covered by #2420.

[therve]

(In [20928])

• (Try to) add error.ProcessExitedAlready behavior to win32
• Add getCommand to return a shell command either in /bin or /usr/bin
• Add lots of documentation to MockOS
• Add more tests around setuid/setgid
• Parametrizes ProcessWriter/ProcessReader on Process for tests

Refs #2341

[Jean-Paul Calderone]

• Typo in MockOS class docstring for WNOHANG - fakin.
• Typo in MockProcessTestCase.tearDown - olfFdesc
• In test_mockPTYSetUidInParent, I guess the line oldPTYProcess = process.PTYProcess would be more appropriate before the try/finally, in case there is an exception, so that the finally suite doesn't encounter a NameError.

The rest seems really great. I think we can probably still do even better on the test coverage front, but this branch already improves that area an amazing amount. With the addition of MockOS, I think it'll be a lot more clear to everyone how we can improve these tests and write better tests for any future changes.

There's a minor conflict in test_process.py now, but it's a no-brainer. It would be nice to merge the branch forward and do a run on the buildslaves though, to make sure there are no surprises.

[therve]

(In [21005]) Correct a few typos.

Refs #2341.

[therve]

(In [21009]) Add tests for reapProcess, typo in process.

Refs #2341.

[therve]

OK builbot seems happy.

[Jean-Paul Calderone]

Looking over the diff again, I noticed one more thing. signalProcess could raise OSError or ProcessExitedAlready in the old implementation and still can in the new implementation. I filed #2787 for addressing this somehow. In the mean time, the test_recvline change really just turns it from one kind of broken to another kind of broken. I guess that code should catch both exception types.

Other than this, I think the branch is ready to be merged.

[therve]

(In [21080]) Manage both OSError and ProcessExitedAlready in tearDown.

Refs #2341.

[therve]

(In [21081]) Merge merge-process-and-ptyprocess-2341-3

Authors: itamarst, therve Reviewer: exarkun Fixes #2341

Create the base class twisted.internet.process._BaseProcess, used to merge most of the code duplicated between Process and PTYProcess. Those classes now inherit from this one. This helped to fix a few bugs, in particular one with setuid management in PTYProcess.