Opened 8 years ago

Closed 7 years ago

#5880 defect closed fixed (fixed)

twisted.internet.process.PTYProcess throws "OSError: [Errno 6] Device not configured" on Mountain Lion

Reported by: faulkner Owned by: Glyph
Priority: normal Milestone:
Component: core Keywords: osx
Cc: Branch: branches/TIOCNOTTY-5880
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description (last modified by Glyph)

Easy to verify this by running trial.

example:
ManholeLoopbackStdio
    testClassDefinition ... Upon execvpe /usr/local/bin/python ['/usr/local/bin/python', '/Users/chris/svn/twisted/twisted/conch/stdio.py', 'twisted.conch.stdio.ConsoleManhole'] in environment id 140257669941472
:Traceback (most recent call last):
  File "/Users/chris/svn/twisted/twisted/internet/process.py", line 418, in _fork
    self._setupChild(**kwargs)
  File "/Users/chris/svn/twisted/twisted/internet/process.py", line 1001, in _setupChild
    os.close(fd)
OSError: [Errno 6] Device not configured

Change History (17)

comment:1 Changed 8 years ago by Glyph

Description: modified (diff)

comment:2 Changed 8 years ago by Glyph

Worth noting that, as a result of this error, the test suite hangs.

comment:3 Changed 8 years ago by Glyph

Keywords: review added

FreeBSD and OS X both have this in 'man 4 pty':

TIOCNOTTY void
    This call is obsolete but left for compatibility.

Linux indicates that the call is still used, and perhaps it's still used on other platforms too, but fixing it causes no tests to fail and allows the existing suite to continue.

comment:4 Changed 8 years ago by Glyph

Branch: branches/TIOCNOTTY-5880

Automation didn't associate a branch; not sure why.

comment:5 Changed 8 years ago by Glyph

Since I did this at a sprint I forced a build on twisted.internet.test.test_process and twisted.test.test_process separately; they're here: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/TIOCNOTTY-5880&num_builds=2

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

Automation didn't associate a branch; not sure why.

1.4.1 (in the second list) on http://twistedmatrix.com/trac/wiki/CommitterCheckList covers this.

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

Keywords: review removed
Owner: set to Glyph

glyph elaborated on irc:

We want to disassociate from the controlling terminal in the subprocess as part of spawnProcess(usePTY=True). In Linux, in order to do that, my understanding is that you need to use both TIOCNOTTY and TIOCSCTTY in order to do that. You have "never" (for Twisted-relevant durations of "never") needed to specify TIOCNOTTY on BSD-like platforms to do that, but it generally doesn't do any harm. Now, in Mountain Lion, there's a bug that causes close() to blow up after TIOCNOTTY is used. This causes PTY support to break. The bug isn't ours, but we can work around it trivially by avoiding the call to TIOCNOTTY which was previously a no-op on this platform anyway. There are other ways to fix it (like ignoring the error on close()) but since just skipping several pointless syscalls doesn't cause the tests to break, I thought that would be a better way to go.

Which seems sensible to me. However, I pointed out:

What I was trying to say is that there are no unit tests for any of the TIOCNOTTY related code. There are unit tests that accidentally invoke that code, but apart from depending on that code not raising random exceptions, they don't depend on the behavior provided in any way. So, my idea is... allocate a pty, launch a process with it. Have that process launch a process with spawnProcess(usePTY=True). assert that the child and the grand child have different ttys.

This sounds like a good idea to me, but of course I haven't actually tried this approach myself, and terminals are often surprising. Hopefully this or at least a similar testing approach will give us good coverage of the desired behavior, though.

comment:9 Changed 7 years ago by Glyph

Further research suggests that, in fact, this code is pointless on all platforms.

We do have test coverage of the intended effect - that the controlling terminal, also known as /dev/tty, is attached to the file descriptor synthesized by spawnProcess, the one that your ProcessProtocol.transport.write will write to. And, if all the code that makes this happen is deleted, twisted.test.test_process.PosixProcessTestCasePTY.testOpeningTTY even fails! (On Linux I do have to hit 'enter' to get it to fail rather than hang, so the test could surely be better, but it is present.)

But I can delete all the TIOCNOTTY code and it makes no difference anywhere.

Quoth the POSIX:

"Upon return the calling process ... shall have no controlling terminal."

In order to achieve the desired, and already tested-for effect, all that is necessary is to (A) call setsid(), and then (B) call ioctl(slavefd, TIOCSCTTY), which we already do after all the shenanigans with TIOCNOTTY. Effectively, what this code is doing right now is starting to daemonize itself, then changing its mind and giving itself a different controlling terminal instead.

So, I'm just going to delete the TIOCNOTTY code instead of what the branch currently does. And, I posit that no new tests are required because it's totally a no-op!

comment:10 Changed 7 years ago by Glyph

(In [35647]) It turns out that none of this is ever necessary, nor has it ever been necessary. Refs #5880

comment:11 Changed 7 years ago by Glyph

Keywords: review added
Owner: Glyph deleted

I forced a build, and it looks good aside from unrelated issues on the XP 32 builder and a red herring on the documentation builder. I think this can land:

$ diffstat
 process.py |   17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

comment:12 Changed 7 years ago by Tom Prince

Owner: set to Tom Prince

comment:13 Changed 7 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Glyph
  • I can confirm that deleting the TIOCSCTTY code (and not the TIOCNOTTY code) cause a test failure in twisted.test.test_process.PosixProcessTestCasePTY.testOpeningTTY (on linux), so the change is adequately covered by tests.
  • That test checks that reading/writing to the controlling terminal in the subprocess gets relayed back to the parent. I can't see anyway for this to happen without trial's tty being hooked up to itself, and even then it is likely to break or hang.
  • I assume this fix has been tested on Mountain Lion.
  • _setupChild could perhaps use an expand docstring.
  • There is no topfile.

comment:14 in reply to:  13 Changed 7 years ago by Glyph

Thanks for the review, Tom. This review is in a bit of a weird format though. You might want to re-read the how to be a good reviewer section of the review process documentation. In particular I'm not sure if you want me to commit this or submit it for re-review. You also haven't really separated out your subjective impressions from the required responses to review feedback, which is always helpful.

Replying to tom.prince:

  • I can confirm that deleting the TIOCSCTTY code (and not the TIOCNOTTY code) cause a test failure in twisted.test.test_process.PosixProcessTestCasePTY.testOpeningTTY (on linux), so the change is adequately covered by tests.

Thanks for verifying that. I guess I don't have to do anything there.

  • That test checks that reading/writing to the controlling terminal in the subprocess gets relayed back to the parent. I can't see anyway for this to happen without trial's tty being hooked up to itself, and even then it is likely to break or hang.

... yes ...

  • I assume this fix has been tested on Mountain Lion.

Yes, it has; I should have stated that explicitly.

  • _setupChild could perhaps use an expand docstring.

OK.

  • There is no topfile.

Uh... no? Here's the topfile in the source browser.

comment:16 Changed 7 years ago by Glyph

Talked to therve about this and he suggests it should be construed as a passing review. So I'll treat it that way.

comment:17 Changed 7 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [35696]) Merge TIOCNOTTY-5880.

Author: glyph

Reviewer: exarkun, tom.prince

Fixes: #5880

Remove useless code that redundantly invokes ioctl(TIOCNOTTY) before calling setsid, that was causing problems on OS X 10.8.

Note: See TracTickets for help on using tickets.