improve error handling robustness for os.execvpe

[ryanpetrello]

see: https://github.com/pexpect/pexpect/issues/512

Improved behavior (with this PR):

[ryanpetrello]

cc @anwilli5 if you're still out there, I'd love your thoughts on this PR, since I think you wrote the original implementation at https://github.com/pexpect/ptyprocess/pull/1

[anwilli5]

Your PR seems reasonable to me. It'd probably be safe to simplify the exception description string to something like '{}:0:{}'.format(cls_name, str(err))

The reason the code caught OSError specifically is because the Python documentation for the os library says:

Note: All functions in this module raise OSError in the case of invalid or inaccessible file names and paths, or other arguments that have the correct type, but are not accepted by the operating system.

and, from the documentation for the exec functions:

Errors will be reported as OSError exceptions.

It looks like those statements aren't as concrete as I interpreted them to be!

[ryanpetrello]

It seems to me that we want to keep the special case for OSError because it allows the errno to be restored explicitly when the exception is rebuilt by the parent. Otherwise, using a hard coded 0 all the time loses that context for OSError. This works because the constructor signature for OSError is simple, and because OSError is a builtin. For other exceptions, falling back to plain old Exception with a nicely formatted message is probably enough to get the point across - we don’t have to worry about importing code, function signatures, side effects of constructing new exception objects, etc...

[ryanpetrello]

@takluyver any thoughts on this one?

[takluyver]

Thanks, this seems like a sensible improvement.

For the unicode error, I'd suggest that we also check for that before forking, i.e. by encoding environment variables and argv in advance. That avoids the complexity of a fork and the machinery to send an error back through a pipe. It could also give clearer error messages (e.g. which thing is causing a unicode error?).

Possibly have a look at the code in the subprocess module. I think this 'error pipe' mechanism is borrowed from there.

[ryanpetrello]

@takluyver,

I'm a bit delayed in my response, but I recently updated a project I work on to support py3, and ran into this all over again. I've updated this PR w/ the changes you requested - any chance you have some time to look them over?

[ryanpetrello]

@takluyver,

Sorry to nag, any chance you've had some time to look this one over?

[ryanpetrello]

@takluyver this is a pretty notable error for Python 3 users. Is there any chance we could get it merged?

[Red-M]

I can merge this but I am very unfamiliar with this code base.

[ryanpetrello]

@takluyver I actually ran into another version of this bug today, which can occur if you specify an env var that has an integer or float as a env var value:

~ python
>>> import pexpect
>>> pexpect.spawn(
...     'sleep', ['1'], cwd='/tmp/', env={'FOO': 1}, ignore_sighup=True,
...     encoding='utf-8', echo=False
... )

# <python hangs>

Traceback (most recent call last):
awx_1        |   File "/venv/awx/lib64/python3.6/site-packages/pexpect/pty_spawn.py", line 303, in _spawn
awx_1        |     cwd=self.cwd, **kwargs)
awx_1        |   File "/venv/awx/lib64/python3.6/site-packages/pexpect/pty_spawn.py", line 314, in _spawnpty
awx_1        |     return ptyprocess.PtyProcess.spawn(args, **kwargs)
awx_1        |   File "/venv/awx/lib64/python3.6/site-packages/ptyprocess/ptyprocess.py", line 353, in spawn
awx_1        |     raise exception
awx_1        | Exception: TypeError: expected str, bytes or os.PathLike object, not float

This PR also fixes that case.

Do you have very strong feelings about the review feedback on this PR? I've found that it definitely addresses the bugs I've found (which are pretty serious, given that they cause pexpect's spawn to hang forever).

[takluyver]

Do you have strong feelings against any of my feedback? I generally expect that when I make suggestions as a project maintainer, they'll get a response, even if that's "I think it's actually better this way because..."

If you're too busy, I'll try to get round to applying my suggestions myself. But I'm pretty busy too, as you might have noticed :wink: .

[ryanpetrello]

Do you have strong feelings against any of my feedback?

@takluyver nope, not especially - just also a lack of time on my end w/ the endless volunteer and open source work I'm doing 😄

Whichever one of us finds time to get to this first 👍