Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

redirect_in/redirect_out should be constrained to windows only #775

Closed
wants to merge 2 commits into from

4 participants

Paul Ivanov Min RK Evan Patterson Fernando Perez
Paul Ivanov
Owner

creating a PIPE for stdin breaks unicode symbols in e.g. !ls magic when inside
qtconsole (at least under Ubuntu)

To verify, make a new file or directory with a unicode character, say mkdir
tmp€

Now in qtconsole, ls will list that directory as tmp���/ in trunk. This PL fixes the
issue.

In Ubuntu 10.04, I see no issues with stdin being invalid when a process is
backgrounded (neither via & at the end of ipython command, nor via
Ctrl-Z followed by bg), and given the nature of redirect_out, and
the Popen bug referenced, I presume this may be a Windows-only issue. So I moved
all of the redirect_in/redirect_out logic to the sys.platform == 'win32'
section.

@epatters - can you take a look at this and verify that it still works on
windows as intended?

update

The original issue described is also solved by #770 without having to remove the stdin PIPE.
Evan says the stdin PIPE solves deadlock issues that arise on OS X, so at least that portion of the change is not windows-only (but is it OS X only?)

Min RK
Owner

I believe this is actually introduced by recent merge of PR #769 (reopened after merge as #770), which was too aggressive about making bytes safe for JSON (we previously considered bytes safe for JSON, which would usually work on posix, because JSON's default is utf8, as is the default terminal encoding on OSX and Linux, but it wasn't actually safe, causing errors on Windows, or if you print latin1 byes on Linux, etc.).

When trying to be safe about encodings, it's rather challenging when we are starting subprocesses that aren't hooked up to any terminals.

Paul Ivanov
Owner
Evan Patterson
Collaborator

@ivanov, this is not Windows-specific. As noted in my comments, the Qt console can deadlock when run as a background process. This happens every time for me under OS X (and has been reported by several others). It may be safe for certain Linux configurations, but it is not safe in general.

I can experiment under Linux, but surely there is a better solution for Unicode support than this?

Min RK
Owner

In these, stdin.encoding is None, and pretty much all of our decoding uses enc=sys.stdin.encoding or sys.getdefaultencoding(), which will return ASCII, so any unicode characters get clobbered.

If we change these cases to sys.stdin.encoding or locale.getpreferredencoding() or sys.getdefaultencoding(), then we should get more sensible, less conservative behavior.

Min RK
Owner

See PR #770 for a fix using locale in between stdin.encoding and getdefaultencoding. It should fix this issue (and should respect codepages on Windows, etc). It addresses how all bytes objects get to the frontend in general, not just from subprocesses.

Paul Ivanov
Owner

ok, with #770, I can confirm that ls with unicode filenames works even with PIPE'd stdin, thanks @minrk

I still think that redirect_out should be constrained to the windows section, given that it checks if its running pythonw.exe

shall i reformulate the PL for that? I may get my hands on a Mac OS X platform tomorrow to look into the deadlock issue @epatters describes.

Evan Patterson
Collaborator

@ivanov, I'm glad Min's fix worked for you. Even so, the stdin redirection is a hack, and I would be glad to see it replaced with something else, so feel free to experiment.

Fernando Perez
Owner

Hey folks, I've just merged #770. I admit I haven't fully grokked the discussion here, what should we do next?

Evan Patterson
Collaborator

I think we can close this, since the PR is not suitable for merging and is really just a workaround for a bug that was apparently fixed (according to Paul's last post).

Fernando Perez
Owner

@ivanov, does that sound good then? Should we just close this one?

Paul Ivanov
Owner

sounds good to me, for now, i'll open another one if i ever get around to tracking down the osx issue Evan described

Paul Ivanov ivanov closed this
Fernando Perez
Owner

Thanks, @ivanov! This helps us keep the PR page reasonably clean so we can focus on the stuff that's really active.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 7, 2011
  1. Paul Ivanov

    PIPE for stdin only needed on windows

    ivanov authored
    Moreover, it breaks unicode in e.g. !ls magic
  2. Paul Ivanov

    removed unused import (atexit)

    ivanov authored
This page is out of date. Refresh to see the latest.
Showing with 34 additions and 34 deletions.
  1. +34 −34 IPython/zmq/entry_point.py
68 IPython/zmq/entry_point.py
View
@@ -3,7 +3,6 @@
"""
# Standard library imports.
-import atexit
import os
import socket
from subprocess import Popen, PIPE
@@ -90,26 +89,6 @@ def base_launch_kernel(code, shell_port=0, iopub_port=0, stdin_port=0, hb_port=0
arguments.append('--ip=%s'%ip)
arguments.extend(extra_arguments)
- # Popen will fail (sometimes with a deadlock) if stdin, stdout, and stderr
- # are invalid. Unfortunately, there is in general no way to detect whether
- # they are valid. The following two blocks redirect them to (temporary)
- # pipes in certain important cases.
-
- # If this process has been backgrounded, our stdin is invalid. Since there
- # is no compelling reason for the kernel to inherit our stdin anyway, we'll
- # place this one safe and always redirect.
- redirect_in = True
- _stdin = PIPE if stdin is None else stdin
-
- # If this process in running on pythonw, we know that stdin, stdout, and
- # stderr are all invalid.
- redirect_out = sys.executable.endswith('pythonw.exe')
- if redirect_out:
- _stdout = PIPE if stdout is None else stdout
- _stderr = PIPE if stderr is None else stderr
- else:
- _stdout, _stderr = stdout, stderr
-
# Spawn a kernel.
if sys.platform == 'win32':
# Create a Win32 event for interrupting the kernel.
@@ -122,6 +101,27 @@ def base_launch_kernel(code, shell_port=0, iopub_port=0, stdin_port=0, hb_port=0
# long time; see http://bugs.python.org/issue706263.
# A cleaner solution to this problem would be to pass os.devnull to
# Popen directly. Unfortunately, that does not work.
+
+ # Popen will fail (sometimes with a deadlock) if stdin, stdout, and stderr
+ # are invalid. Unfortunately, there is in general no way to detect whether
+ # they are valid. The following two blocks redirect them to (temporary)
+ # pipes in certain important cases.
+
+ # If this process has been backgrounded, our stdin is invalid. Since there
+ # is no compelling reason for the kernel to inherit our stdin anyway, we'll
+ # place this one safe and always redirect.
+ redirect_in = True
+ _stdin = PIPE if stdin is None else stdin
+
+ # If this process in running on pythonw, we know that stdin, stdout, and
+ # stderr are all invalid.
+ redirect_out = sys.executable.endswith('pythonw.exe')
+ if redirect_out:
+ _stdout = PIPE if stdout is None else stdout
+ _stderr = PIPE if stderr is None else stderr
+ else:
+ _stdout, _stderr = stdout, stderr
+
if executable.endswith('pythonw.exe'):
if stdout is None:
arguments.append('--no-stdout')
@@ -143,25 +143,25 @@ def base_launch_kernel(code, shell_port=0, iopub_port=0, stdin_port=0, hb_port=0
proc = Popen(arguments + ['--parent=%i'%int(handle)],
stdin=_stdin, stdout=_stdout, stderr=_stderr)
- # Attach the interrupt event to the Popen objet so it can be used later.
+ # Attach the interrupt event to the Popen object so it can be used later.
proc.win32_interrupt_event = interrupt_event
+ # Clean up pipes created to work around windows Popen bug.
+ if redirect_in:
+ if stdin is None:
+ proc.stdin.close()
+ if redirect_out:
+ if stdout is None:
+ proc.stdout.close()
+ if stderr is None:
+ proc.stderr.close()
+
else:
if independent:
proc = Popen(arguments, preexec_fn=lambda: os.setsid(),
- stdin=_stdin, stdout=_stdout, stderr=_stderr)
+ stdin=stdin, stdout=stdout, stderr=stderr)
else:
proc = Popen(arguments + ['--parent=1'],
- stdin=_stdin, stdout=_stdout, stderr=_stderr)
+ stdin=stdin, stdout=stdout, stderr=stderr)
- # Clean up pipes created to work around Popen bug.
- if redirect_in:
- if stdin is None:
- proc.stdin.close()
- if redirect_out:
- if stdout is None:
- proc.stdout.close()
- if stderr is None:
- proc.stderr.close()
-
return proc, shell_port, iopub_port, stdin_port, hb_port
Something went wrong with that request. Please try again.