Skip to content
This repository

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
Collaborator

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
Collaborator
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
Collaborator

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
Collaborator

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 September 15, 2011
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

Showing 2 unique commits by 1 author.

Sep 07, 2011
Paul Ivanov PIPE for stdin only needed on windows
Moreover, it breaks unicode in e.g. !ls magic
e8be13a
Paul Ivanov removed unused import (atexit) 6ebcc13
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 34 additions and 34 deletions. Show diff stats Hide diff stats

  1. 68  IPython/zmq/entry_point.py
68  IPython/zmq/entry_point.py
@@ -3,7 +3,6 @@
3 3
 """
4 4
 
5 5
 # Standard library imports.
6  
-import atexit
7 6
 import os
8 7
 import socket
9 8
 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
90 89
         arguments.append('--ip=%s'%ip)
91 90
     arguments.extend(extra_arguments)
92 91
 
93  
-    # Popen will fail (sometimes with a deadlock) if stdin, stdout, and stderr
94  
-    # are invalid. Unfortunately, there is in general no way to detect whether
95  
-    # they are valid.  The following two blocks redirect them to (temporary)
96  
-    # pipes in certain important cases.
97  
-
98  
-    # If this process has been backgrounded, our stdin is invalid. Since there
99  
-    # is no compelling reason for the kernel to inherit our stdin anyway, we'll
100  
-    # place this one safe and always redirect.
101  
-    redirect_in = True
102  
-    _stdin = PIPE if stdin is None else stdin
103  
-
104  
-    # If this process in running on pythonw, we know that stdin, stdout, and
105  
-    # stderr are all invalid.
106  
-    redirect_out = sys.executable.endswith('pythonw.exe')
107  
-    if redirect_out:        
108  
-        _stdout = PIPE if stdout is None else stdout
109  
-        _stderr = PIPE if stderr is None else stderr
110  
-    else:
111  
-        _stdout, _stderr = stdout, stderr 
112  
-
113 92
     # Spawn a kernel.
114 93
     if sys.platform == 'win32':
115 94
         # 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
122 101
         # long time; see http://bugs.python.org/issue706263.
123 102
         # A cleaner solution to this problem would be to pass os.devnull to
124 103
         # Popen directly. Unfortunately, that does not work.
  104
+
  105
+        # Popen will fail (sometimes with a deadlock) if stdin, stdout, and stderr
  106
+        # are invalid. Unfortunately, there is in general no way to detect whether
  107
+        # they are valid.  The following two blocks redirect them to (temporary)
  108
+        # pipes in certain important cases.
  109
+
  110
+        # If this process has been backgrounded, our stdin is invalid. Since there
  111
+        # is no compelling reason for the kernel to inherit our stdin anyway, we'll
  112
+        # place this one safe and always redirect.
  113
+        redirect_in = True
  114
+        _stdin = PIPE if stdin is None else stdin
  115
+
  116
+        # If this process in running on pythonw, we know that stdin, stdout, and
  117
+        # stderr are all invalid.
  118
+        redirect_out = sys.executable.endswith('pythonw.exe')
  119
+        if redirect_out:
  120
+            _stdout = PIPE if stdout is None else stdout
  121
+            _stderr = PIPE if stderr is None else stderr
  122
+        else:
  123
+            _stdout, _stderr = stdout, stderr
  124
+
125 125
         if executable.endswith('pythonw.exe'):
126 126
             if stdout is None:
127 127
                 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
143 143
             proc = Popen(arguments + ['--parent=%i'%int(handle)],
144 144
                          stdin=_stdin, stdout=_stdout, stderr=_stderr)
145 145
 
146  
-        # Attach the interrupt event to the Popen objet so it can be used later.
  146
+        # Attach the interrupt event to the Popen object so it can be used later.
147 147
         proc.win32_interrupt_event = interrupt_event
148 148
 
  149
+        # Clean up pipes created to work around windows Popen bug.
  150
+        if redirect_in:
  151
+            if stdin is None:
  152
+                proc.stdin.close()
  153
+        if redirect_out:
  154
+            if stdout is None:
  155
+                proc.stdout.close()
  156
+            if stderr is None:
  157
+                proc.stderr.close()
  158
+
149 159
     else:
150 160
         if independent:
151 161
             proc = Popen(arguments, preexec_fn=lambda: os.setsid(),
152  
-                         stdin=_stdin, stdout=_stdout, stderr=_stderr)
  162
+                         stdin=stdin, stdout=stdout, stderr=stderr)
153 163
         else:
154 164
             proc = Popen(arguments + ['--parent=1'],
155  
-                         stdin=_stdin, stdout=_stdout, stderr=_stderr)
  165
+                         stdin=stdin, stdout=stdout, stderr=stderr)
156 166
 
157  
-    # Clean up pipes created to work around Popen bug.
158  
-    if redirect_in:
159  
-        if stdin is None:
160  
-            proc.stdin.close()
161  
-    if redirect_out:
162  
-        if stdout is None:
163  
-            proc.stdout.close()
164  
-        if stderr is None:
165  
-            proc.stderr.close()
166  
-            
167 167
     return proc, shell_port, iopub_port, stdin_port, hb_port
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.