-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't use fork to start the notebook in js tests #5339
Changes from 3 commits
ea201ca
8ea2b62
a265e56
dcd8107
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,16 +19,16 @@ | |
from __future__ import print_function | ||
|
||
import argparse | ||
import json | ||
import multiprocessing.pool | ||
from multiprocessing import Process, Queue | ||
import os | ||
import shutil | ||
import signal | ||
import sys | ||
import subprocess | ||
import time | ||
|
||
from .iptest import have, test_group_names as py_test_group_names, test_sections | ||
from .iptest import have, test_group_names as py_test_group_names, test_sections, StreamCapturer | ||
from IPython.utils.path import compress_user | ||
from IPython.utils.py3compat import bytes_to_str | ||
from IPython.utils.sysinfo import get_sys_info | ||
|
@@ -211,48 +211,77 @@ def setup(self): | |
os.makedirs(os.path.join(self.nbdir.name, os.path.join(u'sub ∂ir2', u'sub ∂ir 1b'))) | ||
|
||
# start the ipython notebook, so we get the port number | ||
self.server_port = 0 | ||
self._init_server() | ||
self.cmd.append('--port=%s' % self.server_port) | ||
|
||
if self.server_port: | ||
self.cmd.append("--port=%i" % self.server_port) | ||
else: | ||
# don't launch tests if the server didn't start | ||
self.cmd = [sys.executable, '-c', 'raise SystemExit(1)'] | ||
|
||
def print_extra_info(self): | ||
print("Running tests with notebook directory %r" % self.nbdir.name) | ||
|
||
@property | ||
def will_run(self): | ||
return all(have[a] for a in ['zmq', 'tornado', 'jinja2', 'casperjs']) | ||
return all(have[a] for a in ['zmq', 'tornado', 'jinja2', 'casperjs', 'sqlite3']) | ||
|
||
def _init_server(self): | ||
"Start the notebook server in a separate process" | ||
self.queue = q = Queue() | ||
self.server = Process(target=run_webapp, args=(q, self.ipydir.name, self.nbdir.name)) | ||
self.server.start() | ||
self.server_port = q.get() | ||
self.server_command = command = [sys.executable, | ||
'-m', 'IPython.html', | ||
'--no-browser', | ||
'--ipython-dir', self.ipydir.name, | ||
'--notebook-dir', self.nbdir.name, | ||
] | ||
# ipc doesn't work on Windows, and darwin has crazy-long temp paths, | ||
# which run afoul of ipc's maximum path length. | ||
if sys.platform.startswith('linux'): | ||
command.append('--KernelManager.transport=ipc') | ||
self.stream_capturer = c = StreamCapturer() | ||
c.start() | ||
self.server = subprocess.Popen(command, stdout=c.writefd, stderr=subprocess.STDOUT) | ||
self.server_info_file = os.path.join(self.ipydir.name, | ||
'profile_default', 'security', 'nbserver-%i.json' % self.server.pid | ||
) | ||
self._wait_for_server() | ||
|
||
def _wait_for_server(self): | ||
"""Wait 30 seconds for the notebook server to start""" | ||
for i in range(300): | ||
if self.server.poll() is not None: | ||
return self._failed_to_start() | ||
if os.path.exists(self.server_info_file): | ||
self._load_server_info() | ||
return | ||
time.sleep(0.1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just for curiosity, why you are using 0.1 resolution in time.sleep? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reason to wait a whole second each time, that just makes things unnecessarily slow. |
||
print("Notebook server-info file never arrived: %s" % self.server_info_file, | ||
file=sys.stderr | ||
) | ||
|
||
def _failed_to_start(self): | ||
"""Notebook server exited prematurely""" | ||
captured = self.stream_capturer.get_buffer().decode('utf-8', 'replace') | ||
print("Notebook failed to start: ", file=sys.stderr) | ||
print(self.server_command) | ||
print(captured, file=sys.stderr) | ||
|
||
def _load_server_info(self): | ||
"""Notebook server started, load connection info from JSON""" | ||
with open(self.server_info_file) as f: | ||
info = json.load(f) | ||
self.server_port = info['port'] | ||
|
||
def cleanup(self): | ||
self.server.terminate() | ||
self.server.join() | ||
self.stream_capturer.halt() | ||
try: | ||
self.server.terminate() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should stop the server before the stream capturer - once the stream capturer has stopped, it's theoretically possible for the server to fill up its stdout/stderr buffer and hang. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moved halt after terminate. |
||
except OSError: | ||
# already dead | ||
pass | ||
self.server.wait() | ||
TestController.cleanup(self) | ||
|
||
def run_webapp(q, ipydir, nbdir, loglevel=0): | ||
"""start the IPython Notebook, and pass port back to the queue""" | ||
import os | ||
import IPython.html.notebookapp as nbapp | ||
import sys | ||
sys.stderr = open(os.devnull, 'w') | ||
server = nbapp.NotebookApp() | ||
args = ['--no-browser'] | ||
args.extend(['--ipython-dir', ipydir, | ||
'--notebook-dir', nbdir, | ||
'--log-level', str(loglevel), | ||
]) | ||
# ipc doesn't work on Windows, and darwin has crazy-long temp paths, | ||
# which run afoul of ipc's maximum path length. | ||
if sys.platform.startswith('linux'): | ||
args.append('--KernelManager.transport=ipc') | ||
server.initialize(args) | ||
# communicate the port number to the parent process | ||
q.put(server.port) | ||
server.start() | ||
|
||
def prepare_controllers(options): | ||
"""Returns two lists of TestController instances, those to run, and those | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we always use the default profile for all testing related things? Is it possible to run tests under another profile? Basically, is this safe to hardcode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that IPYTHONDIR get set, so this won't be the ~/.ipython/profile_default, but /tmp/soma_random_dir/.ipython/profile_default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's always profile_default, but it's never the user's actual profile. IPYTHONDIR is always a newly created temp dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yes, that for the clarification.
On Thu, Mar 13, 2014 at 8:56 PM, Min RK notifications@github.com wrote:
Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com