Opened 15 years ago
Closed 15 years ago
#2341 defect closed fixed (fixed)
Merge twisted.internet.process.PTYProcess and Process
Reported by: | itamarst | Owned by: | |
---|---|---|---|
Priority: | highest | Milestone: | |
Component: | core | Keywords: | |
Cc: | jknight, therve, Jean-Paul Calderone, itamarst | Branch: | |
Author: |
Description
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.
Change History (31)
comment:1 Changed 15 years ago by
Cc: | jknight added |
---|
comment:2 Changed 15 years ago by
Cc: | therve added |
---|---|
Owner: | changed from Glyph to itamarst |
comment:4 Changed 15 years ago by
Owner: | changed from itamarst to therve |
---|
comment:5 Changed 15 years ago by
Owner: | changed from therve to itamarst |
---|
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 ?
comment:6 Changed 15 years ago by
Owner: | changed from itamarst to therve |
---|
This needs some new tests, especially for PTY support. See the changes for applicable things; the changes probably shouldn't be in the code.
comment:7 Changed 15 years ago by
Cc: | Jean-Paul Calderone itamarst added |
---|---|
Keywords: | review added |
Owner: | therve deleted |
Priority: | normal → highest |
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.
comment:8 Changed 15 years ago by
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.
comment:9 follow-up: 20 Changed 15 years ago by
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.
comment:10 follow-up: 11 Changed 15 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
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__
andPTYProcess.__init__
still have a lot of overlap and slightly differing behavior. eg,PTYProcess
callsFileDescriptor.__init__
,Process
doesn't. A bit more refactoring in this area might not be amiss.- The change in
PTYProcess.doRead
to calloutReceived
instead ofchildDataReceived
doesn't seem correct to me. What's up there?
comment:11 Changed 15 years ago by
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__
andPTYProcess.__init__
still have a lot of overlap and slightly differing behavior. eg,PTYProcess
callsFileDescriptor.__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 calloutReceived
instead ofchildDataReceived
doesn't seem correct to me. What's up there?
My bad, I just took a shortcut. I revert that.
comment:12 Changed 15 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
win32 buildbot is green, so I put it back in review.
comment:14 Changed 15 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
- 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 namedexecutable
- 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.
- in
- registerReapProcessHandler
- 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.
comment:15 Changed 15 years ago by
JP, thanks for looking at this. As part of review, can you look at the changes noted in a comment 9 above? Thanks.
comment:18 Changed 15 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
OK I think I tackled the points. I renamed command to executable everywhere to be consistent. Thanks for the review.
comment:19 Changed 15 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
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.
comment:20 Changed 15 years ago by
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.
comment:21 Changed 15 years ago by
"Now raises ProcessExitedAlready in signalProcess" is, I think, now covered by #2420.
comment:22 Changed 15 years ago by
comment:23 Changed 15 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
comment:24 Changed 15 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
- 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 aNameError
.
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.
comment:26 Changed 15 years ago by
comment:27 Changed 15 years ago by
Keywords: | review added |
---|---|
Owner: | therve deleted |
OK builbot seems happy.
comment:28 Changed 15 years ago by
Keywords: | review removed |
---|---|
Owner: | set to therve |
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.
comment:29 Changed 15 years ago by
comment:30 Changed 15 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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.
comment:31 Changed 11 years ago by
Owner: | therve deleted |
---|
OK now #2419 is in, we should move os.kill handling out of here. We still agree on doing this one before #2420 ?