improve error handling robustness for os.execvpe #47
Conversation
ryanpetrello
force-pushed
the
fix-execvpe-unicode-errors
branch
from
867a388 to
3399dd7
Compare
|
Your PR seems reasonable to me. It'd probably be safe to simplify the exception description string to something like The reason the code caught OSError specifically is because the Python documentation for the os library says:
and, from the documentation for the exec functions:
It looks like those statements aren't as concrete as I interpreted them to be! |
|
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... |
|
@takluyver any thoughts on this one? |
|
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
force-pushed
the
fix-execvpe-unicode-errors
branch
2 times, most recently
from
e0bf942 to
978ada4
Compare
ryanpetrello
force-pushed
the
fix-execvpe-unicode-errors
branch
from
978ada4 to
3e4bbba
Compare
|
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? |
|
Sorry to nag, any chance you've had some time to look this one over? |
|
@takluyver this is a pretty notable error for Python 3 users. Is there any chance we could get it merged? |
|
I can merge this but I am very unfamiliar with this code base. |
| @@ -206,6 +206,19 @@ def spawn( | |||
| if not isinstance(argv, (list, tuple)): | |||
| raise TypeError("Expected a list or tuple for argv, got %r" % argv) | |||
|
|
|||
| # env vars must be utf-8 encoded strings | |||
There was a problem hiding this comment.
Always UTF-8? Or should we be trying to apply a locale-dependent encoding?
There was a problem hiding this comment.
On reflection, unless this comment makes you go "aha, yes, I know just how we should do this!", let's stick to UTF-8 encoding and see if anyone complains. It might not be 100% right, but it's easy to understand, and people who really need something else can still pass environment variables as bytes.
| if isinstance(env, dict): | ||
| _text_type = str | ||
| if not PY3: | ||
| _text_type = unicode |
There was a problem hiding this comment.
Let's put this compatibility bit in the existing if PY3: block at the top of the file.
| _text_type = unicode | ||
| for k, v in env.items(): | ||
| if isinstance(v, _text_type): | ||
| env[k] = v.encode('utf-8') |
There was a problem hiding this comment.
IIRC modifying a dict while iterating over it is generally considered a bad idea. Maybe it's better to create a new dict env_b instead.
| tosend = 'OSError:{}:{}'.format(err.errno, str(err)) | ||
| else: | ||
| cls_name = err.__class__.__name__ | ||
| tosend = 'Exception:0:{}: {}'.format(cls_name, str(err)) |
There was a problem hiding this comment.
Following the exception handling around preexec_fn(), maybe this should do '{}:0:{}' (e.g. 'UnicodeEncodeError:0:blah'.
|
@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 floatThis 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 |
|
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 😉 . |
@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 👍 |
see: pexpect/pexpect#512
Improved behavior (with this PR):