From 32cfde78822b2d8758069fa980155d526933edb5 Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Thu, 12 Dec 2024 20:03:46 +0800 Subject: [PATCH 1/9] properly close OutStream and various fixes --- ipykernel/embed.py | 1 + ipykernel/iostream.py | 4 +-- ipykernel/kernelapp.py | 65 ++++++++++++++++++++++++++++++++++-------- 3 files changed, 56 insertions(+), 14 deletions(-) diff --git a/ipykernel/embed.py b/ipykernel/embed.py index 3e4abd39..ad22e2a1 100644 --- a/ipykernel/embed.py +++ b/ipykernel/embed.py @@ -55,3 +55,4 @@ def embed_kernel(module=None, local_ns=None, **kwargs): app.kernel.user_ns = local_ns app.shell.set_completer_frame() # type:ignore[union-attr] app.start() + app.close() diff --git a/ipykernel/iostream.py b/ipykernel/iostream.py index d8171017..ee2b051e 100644 --- a/ipykernel/iostream.py +++ b/ipykernel/iostream.py @@ -595,8 +595,8 @@ def close(self): self._should_watch = False # thread won't wake unless there's something to read # writing something after _should_watch will not be echoed - os.write(self._original_stdstream_fd, b"\0") - if self.watch_fd_thread is not None: + if self.watch_fd_thread is not None and self.watch_fd_thread.is_alive(): + os.write(self._original_stdstream_fd, b"\0") self.watch_fd_thread.join() # restore original FDs os.dup2(self._original_stdstream_copy, self._original_stdstream_fd) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index 66b750b2..44e3bafa 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -397,6 +397,9 @@ def init_heartbeat(self): self.heartbeat.start() def close(self): + if self.closed: + return + self.closed = True """Close zmq sockets in an orderly fashion""" # un-capture IO before we start closing channels self.reset_io() @@ -471,33 +474,45 @@ def log_connection_info(self): def init_blackhole(self): """redirects stdout/stderr to devnull if necessary""" if self.no_stdout or self.no_stderr: - blackhole = open(os.devnull, "w") # noqa: SIM115 + # keep reference around so that it would not accidentally close the pipe fds + self._blackhole = open(os.devnull, "w") # noqa: SIM115 if self.no_stdout: - sys.stdout = sys.__stdout__ = blackhole # type:ignore[misc] + if sys.stdout is not None: + sys.stdout.flush() + sys.stdout = self._blackhole # type:ignore[misc] if self.no_stderr: - sys.stderr = sys.__stderr__ = blackhole # type:ignore[misc] + if sys.stderr is not None: + sys.stderr.flush() + sys.stderr = self._blackhole # type:ignore[misc] def init_io(self): """Redirect input streams and set a display hook.""" if self.outstream_class: outstream_factory = import_item(str(self.outstream_class)) - if sys.stdout is not None: - sys.stdout.flush() - e_stdout = None if self.quiet else sys.__stdout__ - e_stderr = None if self.quiet else sys.__stderr__ + e_stdout = None if self.quiet else sys.stdout + e_stderr = None if self.quiet else sys.stderr if not self.capture_fd_output: outstream_factory = partial(outstream_factory, watchfd=False) + if sys.stdout is not None: + sys.stdout.flush() sys.stdout = outstream_factory(self.session, self.iopub_thread, "stdout", echo=e_stdout) + if sys.stderr is not None: sys.stderr.flush() sys.stderr = outstream_factory(self.session, self.iopub_thread, "stderr", echo=e_stderr) + if hasattr(sys.stderr, "_original_stdstream_copy"): for handler in self.log.handlers: - if isinstance(handler, StreamHandler) and (handler.stream.buffer.fileno() == 2): - self.log.debug("Seeing logger to stderr, rerouting to raw filedescriptor.") + if (isinstance(handler, StreamHandler) + and (buffer := getattr(handler.stream, "buffer")) + and (fileno := getattr(buffer, "fileno")) + and fileno() == sys.stderr._original_stdstream_fd): + self.log.debug( + "Seeing logger to stderr, rerouting to raw filedescriptor." + ) handler.stream = TextIOWrapper( FileIO( @@ -517,9 +532,32 @@ def reset_io(self): restores state after init_io """ - sys.stdout = sys.__stdout__ - sys.stderr = sys.__stderr__ - sys.displayhook = sys.__displayhook__ + stdout, stderr, displayhook = sys.stdout, sys.stderr, sys.displayhook + sys.stdout, sys.stderr, sys.displayhook = self._original_io + if (finish_displayhook := getattr(displayhook, "finish_displayhook", + None)): + finish_displayhook() + if hasattr(sys.stderr, "_original_stdstream_copy"): + for handler in self.log.handlers: + if (isinstance(handler, StreamHandler) + and (buffer := getattr(handler.stream, "buffer")) + and (fileno := getattr(buffer, "fileno")) + and fileno() == sys.stderr._original_stdstream_copy): + self.log.debug( + "Seeing logger to raw filedescriptor, rerouting back to stderr" + ) + + handler.stream = TextIOWrapper( + FileIO( + sys.stderr._original_stdstream_fd, + "w", + ) + ) + stderr.close() + stdout.close() + if self._blackhole: + # already closed by above but no harm calling again + self._blackhole.close() def patch_io(self): """Patch important libraries that can't handle sys.stdout forwarding""" @@ -693,6 +731,9 @@ def init_pdb(self): @catch_config_error def initialize(self, argv=None): """Initialize the application.""" + self.closed = False + self._blackhole = None + self._original_io = sys.stdout, sys.stderr, sys.displayhook self._init_asyncio_patch() super().initialize(argv) if self.subapp is not None: From c3a67fbfe2375a0e1a556bf4a91600ce0787563c Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Wed, 18 Dec 2024 17:17:01 +0800 Subject: [PATCH 2/9] add from __future__ import annotations for Python 3.8 --- ipykernel/_version.py | 2 ++ ipykernel/inprocess/channels.py | 1 + ipykernel/pickleutil.py | 2 ++ ipykernel/thread.py | 2 ++ 4 files changed, 7 insertions(+) diff --git a/ipykernel/_version.py b/ipykernel/_version.py index 5907d150..b4c5b1da 100644 --- a/ipykernel/_version.py +++ b/ipykernel/_version.py @@ -1,6 +1,8 @@ """ store the current version info of the server. """ +from __future__ import annotations + import re # Version string must appear intact for hatch versioning diff --git a/ipykernel/inprocess/channels.py b/ipykernel/inprocess/channels.py index 4c01c5bc..a886f6c8 100644 --- a/ipykernel/inprocess/channels.py +++ b/ipykernel/inprocess/channels.py @@ -2,6 +2,7 @@ # Copyright (c) IPython Development Team. # Distributed under the terms of the Modified BSD License. +from __future__ import annotations from jupyter_client.channelsabc import HBChannelABC diff --git a/ipykernel/pickleutil.py b/ipykernel/pickleutil.py index 4ffa5262..15fc0e67 100644 --- a/ipykernel/pickleutil.py +++ b/ipykernel/pickleutil.py @@ -2,6 +2,8 @@ # Copyright (c) IPython Development Team. # Distributed under the terms of the Modified BSD License. +from __future__ import annotations + import copy import pickle import sys diff --git a/ipykernel/thread.py b/ipykernel/thread.py index 40509ece..b3b33d32 100644 --- a/ipykernel/thread.py +++ b/ipykernel/thread.py @@ -1,4 +1,6 @@ """Base class for threads.""" +from __future__ import annotations + import typing as t from threading import Event, Thread From e80b5e6bac7b2ea4ae8cb4f4ad83264cb516c26b Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Wed, 18 Dec 2024 17:20:43 +0800 Subject: [PATCH 3/9] add closed attribute to IPKernelApp --- ipykernel/kernelapp.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index 44e3bafa..2ab80231 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -204,6 +204,10 @@ def abs_connection_file(self): Interrupt this process when the parent is signaled. """, ).tag(config=True) + closed = Bool( + True, + help="whether this is closed", + ) def init_crash_handler(self): """Initialize the crash handler.""" @@ -532,6 +536,8 @@ def reset_io(self): restores state after init_io """ + if self.closed: + return stdout, stderr, displayhook = sys.stdout, sys.stderr, sys.displayhook sys.stdout, sys.stderr, sys.displayhook = self._original_io if (finish_displayhook := getattr(displayhook, "finish_displayhook", From 86c68736a40d279f19543715eda989abcd2f9f7f Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Wed, 18 Dec 2024 17:21:30 +0800 Subject: [PATCH 4/9] make closing of custom stdout and stderr more strict --- ipykernel/kernelapp.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index 2ab80231..7b26a42c 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -559,10 +559,11 @@ def reset_io(self): "w", ) ) - stderr.close() - stdout.close() + if hasattr(sys.stderr, "_original_stdstream_copy"): + stderr.close() + if hasattr(sys.stdout, "_original_stdstream_copy"): + stdout.close() if self._blackhole: - # already closed by above but no harm calling again self._blackhole.close() def patch_io(self): From 35a260b3418d4416b65d7caf3fffdb7ff377deee Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 12 Dec 2024 15:30:48 +0000 Subject: [PATCH 5/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- ipykernel/kernelapp.py | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index 7b26a42c..a83753e8 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -510,13 +510,13 @@ def init_io(self): if hasattr(sys.stderr, "_original_stdstream_copy"): for handler in self.log.handlers: - if (isinstance(handler, StreamHandler) - and (buffer := getattr(handler.stream, "buffer")) - and (fileno := getattr(buffer, "fileno")) - and fileno() == sys.stderr._original_stdstream_fd): - self.log.debug( - "Seeing logger to stderr, rerouting to raw filedescriptor." - ) + if ( + isinstance(handler, StreamHandler) + and (buffer := handler.stream.buffer) + and (fileno := buffer.fileno) + and fileno() == sys.stderr._original_stdstream_fd + ): + self.log.debug("Seeing logger to stderr, rerouting to raw filedescriptor.") handler.stream = TextIOWrapper( FileIO( @@ -540,18 +540,17 @@ def reset_io(self): return stdout, stderr, displayhook = sys.stdout, sys.stderr, sys.displayhook sys.stdout, sys.stderr, sys.displayhook = self._original_io - if (finish_displayhook := getattr(displayhook, "finish_displayhook", - None)): + if finish_displayhook := getattr(displayhook, "finish_displayhook", None): finish_displayhook() if hasattr(sys.stderr, "_original_stdstream_copy"): for handler in self.log.handlers: - if (isinstance(handler, StreamHandler) - and (buffer := getattr(handler.stream, "buffer")) - and (fileno := getattr(buffer, "fileno")) - and fileno() == sys.stderr._original_stdstream_copy): - self.log.debug( - "Seeing logger to raw filedescriptor, rerouting back to stderr" - ) + if ( + isinstance(handler, StreamHandler) + and (buffer := handler.stream.buffer) + and (fileno := buffer.fileno) + and fileno() == sys.stderr._original_stdstream_copy + ): + self.log.debug("Seeing logger to raw filedescriptor, rerouting back to stderr") handler.stream = TextIOWrapper( FileIO( From 4df22dcc7f236936b85265ff34672bc8798becce Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Wed, 18 Dec 2024 17:35:10 +0800 Subject: [PATCH 6/9] fix reset_io not actually running --- ipykernel/kernelapp.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index a83753e8..8a32ebe2 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -536,8 +536,6 @@ def reset_io(self): restores state after init_io """ - if self.closed: - return stdout, stderr, displayhook = sys.stdout, sys.stderr, sys.displayhook sys.stdout, sys.stderr, sys.displayhook = self._original_io if finish_displayhook := getattr(displayhook, "finish_displayhook", None): From 2e9f6017786959e98109ca6c627aec0fe13dc3e0 Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Wed, 18 Dec 2024 18:26:53 +0800 Subject: [PATCH 7/9] multiple fixes --- ipykernel/kernelapp.py | 34 +++++++++++++++++++--------------- tests/test_kernelapp.py | 1 + 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index 8a32ebe2..dec16d38 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -151,6 +151,10 @@ class IPKernelApp(BaseIPythonApplication, InteractiveShellApp, ConnectionFileMix _ports = Dict() + _original_io = Any() + _io_modified = Bool(False) + _blackhole = Any() + subcommands = { "install": ( "ipykernel.kernelspec.InstallIPythonKernelSpecApp", @@ -204,10 +208,6 @@ def abs_connection_file(self): Interrupt this process when the parent is signaled. """, ).tag(config=True) - closed = Bool( - True, - help="whether this is closed", - ) def init_crash_handler(self): """Initialize the crash handler.""" @@ -401,9 +401,6 @@ def init_heartbeat(self): self.heartbeat.start() def close(self): - if self.closed: - return - self.closed = True """Close zmq sockets in an orderly fashion""" # un-capture IO before we start closing channels self.reset_io() @@ -477,6 +474,7 @@ def log_connection_info(self): def init_blackhole(self): """redirects stdout/stderr to devnull if necessary""" + self._save_io() if self.no_stdout or self.no_stderr: # keep reference around so that it would not accidentally close the pipe fds self._blackhole = open(os.devnull, "w") # noqa: SIM115 @@ -491,6 +489,7 @@ def init_blackhole(self): def init_io(self): """Redirect input streams and set a display hook.""" + self._save_io() if self.outstream_class: outstream_factory = import_item(str(self.outstream_class)) @@ -531,34 +530,42 @@ def init_io(self): self.patch_io() + def _save_io(self): + if not self._io_modified: + self._original_io = sys.stdout, sys.stderr, sys.displayhook + self._io_modified = True + def reset_io(self): """restore original io restores state after init_io """ + if not self._io_modified: + return stdout, stderr, displayhook = sys.stdout, sys.stderr, sys.displayhook sys.stdout, sys.stderr, sys.displayhook = self._original_io + self._io_modified = False if finish_displayhook := getattr(displayhook, "finish_displayhook", None): finish_displayhook() - if hasattr(sys.stderr, "_original_stdstream_copy"): + if hasattr(stderr, "_original_stdstream_copy"): for handler in self.log.handlers: if ( isinstance(handler, StreamHandler) and (buffer := handler.stream.buffer) and (fileno := buffer.fileno) - and fileno() == sys.stderr._original_stdstream_copy + and fileno() == stderr._original_stdstream_copy ): self.log.debug("Seeing logger to raw filedescriptor, rerouting back to stderr") handler.stream = TextIOWrapper( FileIO( - sys.stderr._original_stdstream_fd, + stderr._original_stdstream_fd, "w", ) ) - if hasattr(sys.stderr, "_original_stdstream_copy"): + if hasattr(stderr, "_original_stdstream_copy"): stderr.close() - if hasattr(sys.stdout, "_original_stdstream_copy"): + if hasattr(stdout, "_original_stdstream_copy"): stdout.close() if self._blackhole: self._blackhole.close() @@ -735,9 +742,6 @@ def init_pdb(self): @catch_config_error def initialize(self, argv=None): """Initialize the application.""" - self.closed = False - self._blackhole = None - self._original_io = sys.stdout, sys.stderr, sys.displayhook self._init_asyncio_patch() super().initialize(argv) if self.subapp is not None: diff --git a/tests/test_kernelapp.py b/tests/test_kernelapp.py index 05f6e557..ec91687f 100644 --- a/tests/test_kernelapp.py +++ b/tests/test_kernelapp.py @@ -31,6 +31,7 @@ def test_blackhole(): app.no_stderr = True app.no_stdout = True app.init_blackhole() + app.close() def test_start_app(): From 11ad317d266410b0d95ada05120415c427bf8026 Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Wed, 18 Dec 2024 23:09:24 +0800 Subject: [PATCH 8/9] various fixes --- ipykernel/iostream.py | 10 ++++------ ipykernel/kernelapp.py | 45 ++++++++++++++++++------------------------ 2 files changed, 23 insertions(+), 32 deletions(-) diff --git a/ipykernel/iostream.py b/ipykernel/iostream.py index ee2b051e..62a29a2f 100644 --- a/ipykernel/iostream.py +++ b/ipykernel/iostream.py @@ -398,8 +398,8 @@ def fileno(self): """ Things like subprocess will peak and write to the fileno() of stderr/stdout. """ - if getattr(self, "_original_stdstream_copy", None) is not None: - return self._original_stdstream_copy + if getattr(self, "_original_stdstream_fd", None) is not None: + return self._original_stdstream_fd msg = "fileno" raise io.UnsupportedOperation(msg) @@ -527,10 +527,7 @@ def __init__( # echo on the _copy_ we made during # this is the actual terminal FD now echo = io.TextIOWrapper( - io.FileIO( - self._original_stdstream_copy, - "w", - ) + io.FileIO(self._original_stdstream_copy, "w", closefd=False) ) self.echo = echo else: @@ -598,6 +595,7 @@ def close(self): if self.watch_fd_thread is not None and self.watch_fd_thread.is_alive(): os.write(self._original_stdstream_fd, b"\0") self.watch_fd_thread.join() + self.echo = None # restore original FDs os.dup2(self._original_stdstream_copy, self._original_stdstream_fd) os.close(self._original_stdstream_copy) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index dec16d38..0addeab0 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -152,6 +152,7 @@ class IPKernelApp(BaseIPythonApplication, InteractiveShellApp, ConnectionFileMix _ports = Dict() _original_io = Any() + _log_map = Any() _io_modified = Bool(False) _blackhole = Any() @@ -511,18 +512,16 @@ def init_io(self): for handler in self.log.handlers: if ( isinstance(handler, StreamHandler) - and (buffer := handler.stream.buffer) - and (fileno := buffer.fileno) + and (buffer := getattr(handler.stream, "buffer", None)) + and (fileno := getattr(buffer, "fileno", None)) and fileno() == sys.stderr._original_stdstream_fd ): self.log.debug("Seeing logger to stderr, rerouting to raw filedescriptor.") - - handler.stream = TextIOWrapper( - FileIO( - sys.stderr._original_stdstream_copy, - "w", - ) + io_wrapper = TextIOWrapper( + FileIO(sys.stderr._original_stdstream_copy, "w", closefd=False) ) + self._log_map[id(io_wrapper)] = handler.stream + handler.stream = io_wrapper if self.displayhook_class: displayhook_factory = import_item(str(self.displayhook_class)) self.displayhook = displayhook_factory(self.session, self.iopub_socket) @@ -533,6 +532,7 @@ def init_io(self): def _save_io(self): if not self._io_modified: self._original_io = sys.stdout, sys.stderr, sys.displayhook + self._log_map = {} self._io_modified = True def reset_io(self): @@ -544,29 +544,22 @@ def reset_io(self): return stdout, stderr, displayhook = sys.stdout, sys.stderr, sys.displayhook sys.stdout, sys.stderr, sys.displayhook = self._original_io + self._original_io = None self._io_modified = False if finish_displayhook := getattr(displayhook, "finish_displayhook", None): finish_displayhook() if hasattr(stderr, "_original_stdstream_copy"): for handler in self.log.handlers: - if ( - isinstance(handler, StreamHandler) - and (buffer := handler.stream.buffer) - and (fileno := buffer.fileno) - and fileno() == stderr._original_stdstream_copy - ): - self.log.debug("Seeing logger to raw filedescriptor, rerouting back to stderr") - - handler.stream = TextIOWrapper( - FileIO( - stderr._original_stdstream_fd, - "w", - ) - ) - if hasattr(stderr, "_original_stdstream_copy"): - stderr.close() - if hasattr(stdout, "_original_stdstream_copy"): - stdout.close() + if orig_stream := self._log_map.get(id(handler.stream)): + self.log.debug("Seeing modified logger, rerouting back to stderr") + handler.stream = orig_stream + self._log_map = None + if self.outstream_class: + outstream_factory = import_item(str(self.outstream_class)) + if isinstance(stderr, outstream_factory): + stderr.close() + if isinstance(stdout, outstream_factory): + stdout.close() if self._blackhole: self._blackhole.close() From c08f6749e2e665adfb7c7202ad0d42fe074ad34f Mon Sep 17 00:00:00 2001 From: limwz01 <117669574+limwz01@users.noreply.github.com> Date: Wed, 18 Dec 2024 23:20:04 +0800 Subject: [PATCH 9/9] linting fix --- ipykernel/kernelapp.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ipykernel/kernelapp.py b/ipykernel/kernelapp.py index 0addeab0..453cd99e 100644 --- a/ipykernel/kernelapp.py +++ b/ipykernel/kernelapp.py @@ -482,11 +482,11 @@ def init_blackhole(self): if self.no_stdout: if sys.stdout is not None: sys.stdout.flush() - sys.stdout = self._blackhole # type:ignore[misc] + sys.stdout = self._blackhole if self.no_stderr: if sys.stderr is not None: sys.stderr.flush() - sys.stderr = self._blackhole # type:ignore[misc] + sys.stderr = self._blackhole def init_io(self): """Redirect input streams and set a display hook.""" @@ -514,7 +514,7 @@ def init_io(self): isinstance(handler, StreamHandler) and (buffer := getattr(handler.stream, "buffer", None)) and (fileno := getattr(buffer, "fileno", None)) - and fileno() == sys.stderr._original_stdstream_fd + and fileno() == sys.stderr._original_stdstream_fd # type:ignore[attr-defined] ): self.log.debug("Seeing logger to stderr, rerouting to raw filedescriptor.") io_wrapper = TextIOWrapper(