Skip to content

Commit

Permalink
python: on ProtocolChannel close, close the transport
Browse files Browse the repository at this point in the history
Right now ProtocolChannel basically ignores "close" from the user.  The
correct this to do in this case is to ask the transport to close.  The
"close" message will be sent back to the user once we see our
.connection_lost() method called by the transport.

Doing this means that we no longer need to worry about killing the
subprocess from the stream channel — it happens as part of .close() on
the SubprocessTransport.
  • Loading branch information
allisonkarlitskaya committed Feb 16, 2023
1 parent ce4e755 commit b655ab8
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 13 deletions.
4 changes: 4 additions & 0 deletions src/cockpit/channel.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,10 @@ def do_done(self) -> None:
if self._transport.can_write_eof():
self._transport.write_eof()

def do_close(self) -> None:
if self._transport is not None:
self._transport.close()

def data_received(self, data: bytes) -> None:
assert self._transport is not None
if not self.send_data(data):
Expand Down
13 changes: 0 additions & 13 deletions src/cockpit/channels/stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,19 +107,6 @@ def do_options(self, options):
if window is not None:
self._transport.set_window_size(**window)

def do_close(self):
assert isinstance(self._transport, SubprocessTransport)
pid = self._transport.get_pid()
# avoid calling .terminate(), as that will try to read the process' exit code and race with
# asyncio's ChildWatcher (which also wants to wait() the process); the cockpit.spawn() API
# already ensures that the process is valid before even calling do_close.
try:
os.kill(pid, signal.SIGTERM)
logger.debug('Received close signal from peer, SIGTERMed process %i', pid)
except ProcessLookupError:
# already gone? fine!
logger.debug('Received close signal from peer, but process %i is already gone', pid)

async def create_transport(self, loop: asyncio.AbstractEventLoop, options: Dict[str, object]) -> SubprocessTransport:
args = options['spawn']

Expand Down

0 comments on commit b655ab8

Please sign in to comment.