bugfix telnet server: prompt app not cancelled when client close connection

[wuf]

If client closes the telnet connection (either due to user typing
"Ctrl-] then 'quit'" or due to network shutdown) in middle of the
prompt application, the application will hang forever and resources
maintained in TelnetServer.connections and
TelnetServer._application_tasks are not released.

[jonathanslenders]

Hi @wuf,

Thanks for reporting the issue and proposing this patch!

The underlying issue is more complex, and it looks like there are a couple of bugs. Some things here that I notice:

• I noticed that some of the telnet-related examples in the examples/ folder are not up to date and don't run as-is. This should be simple to fix.
• The more I learn about structured concurrency, the more I think it's the right approach for cancellation in this case. Anything spawned as part of a connection should be spawned as part of a task group, for which the lifetime doesn't extend the lifetime of the connection. For now, however, I don't want to depend on anyio or have a task group implementation in prompt_toolkit, but I regret having a separate .start() and .stop() method in TelnetServer instead of a single .run() coroutine will be stopped through cancellation. It's a breaking change that I hope to fix it at some point.
• Cancelling the interact task at the end of the input is not necessarily the right thing to do. The application should be allowed to send some final output after the input terminates. So, instead, we want to raise EOFError when more input is asked for if the input is closed (this doesn't happen because of the next point).
• It looks like there is a bug in PosixPipeInput. The .close() method closes both the read and write end of the pipe. Closing the read-end causes the attached callback in .vt100.py -> _attached_input() to never wake up. Instead, we should only close the write-end, after which the loop wakes up, calls the callback for the read-end of the pipe where we see that the input is closed, and EOFError will be raised because of that. However, not closing the read-end of the pipe means that it has to be closed elsewhere, so PosixPipeInput should probably become a context manager. That is a breaking change.
• In telnet/server.py, in the run_application() method, it looks like exceptions that are caught, are reraised. This includes an EOFError that was raised in the interact coroutine. We should probably not be reraising exceptions here.

I hope this is sufficient as feedback to set the direction. I can't merge it as-is unfortunately. I'll see whether I can already take care of some bullet points here.
By the way, thanks for the "Ctrl-] then 'quit'" trick. I didn't know about that. This is very useful for testing.

[jonathanslenders]

Hi @wuf,

This is my attempt to solve the issue related to closing the connection: #1586
(It does not deal with all issues yet, and still needs some testing.)

[wuf]

Hi @jonathanslenders,

thanks for the quick reply. Yes, I totally agree with your points and this should be fixed at PosixPipeInput. I once raised the PosixPipe issue #1522 to report this but I can only provide an ugly demo at that time.

Gald that this issue is finally noticed and a mature fix is provided by you. I will try it in my application, thanks.

[wuf]

@jonathanslenders ,

Your patch works, below are a few things that I noticed,

• how about adding __enter__() and __exit__() to turn PosixPipeInput instances into context manager? Then the changes would be compatible to existing code.
• another issue is that when pipe is closed, renderer still tries to write/flush stdout which causes several below warning messages. In my previous patch, I added a _closed flag to _ConnectionStdout to address this. Or maybe we can just lower the logging level?

WARNING:prompt_toolkit.contrib.telnet:Couldn't send data over socket: [Errno 9] Bad file descriptor

[jonathanslenders]

@wuf: Thanks for the very quick reply!

• About __enter__ and __exit__. In general, I started to avoid those for two reasons. First, they don't require that the object will be used as a context manager. Secondly, state (local variables) is often passed from the __enter__ to the __exit__ phase. It doesn't really apply here, but this experience and what I learned about structured concurrency, makes me only want to write context managers anymore using contextlib.contextmanager and contextlib.asynccontextmanager. You have a point about backwards compatibility, but it's hard to guarante that these objects will remain usable without context manager. They will leak file descriptors in any case.

• About the second issue. I just fixed that, exactly like you did.

[jonathanslenders]

@wuf: I merged my branch (I made you co-author in the commit message). Feel free to still comment, if there is anything.

[wuf]

@jonathanslenders,

• the _closed flag wasn't taking effects
• it feels that the EOFError traceback is little bit annoying: closing telnet connection is a normal user operation, and we already has the printing "Connection closed by client" for it.

how about below patch

diff --git a/prompt_toolkit/contrib/telnet/server.py b/prompt_toolkit/contrib/telnet/server.py index 035239d2..227fc1fe 100644 --- a/prompt_toolkit/contrib/telnet/server.py +++ b/prompt_toolkit/contrib/telnet/server.py @@ -99,7 +99,8 @@ class _ConnectionStdout: def flush(self) -> None: try: - self._connection.send(b"".join(self._buffer)) + if not self._closed: + self._connection.send(b"".join(self._buffer)) except OSError as e: logger.warning("Couldn't send data over socket: %s" % e) @@ -355,6 +356,9 @@ class TelnetServer: finally: self.connections.remove(connection) logger.info("Stopping interaction %r %r", *addr) + except EOFError: + # caused by "Connection closed by client" + pass except BaseException as e: print("Got %s" % type(e).__name__, e) import traceback 

[jonathanslenders]

@wuf: Thanks for the feedback and patch! I merged it like this: #1589

[jonathanslenders]

I'm closing this. Thanks again @wuf!