sibson / vncdotool

A command line VNC client and python library
Other
451 stars 120 forks source link

Programs using api.connect hang unless api.shutdown is explicitly called before exit #288

Open cdleonard opened 3 months ago

cdleonard commented 3 months ago

Please include the following information:

Client: vncdotool version: 1.2.0 Server: TigerVNC Server version 1.13.1, built Jan 30 2024 00:00:00

Run the following program:

#! /usr/bin/env python3
import sys
import threading

import vncdotool.api

def main():
    server = sys.argv[1]
    passwd = sys.argv[2]
    with vncdotool.api.connect(server, passwd):
        pass
    for th in threading.enumerate():
        sys.stdout.write(f"thread name {th.name} daemon {th.daemon}\n")

if __name__ == "__main__":
    main()

Expected result: shuts down nicely

Actual result:

thread name MainThread daemon False
thread name Twisted Reactor daemon True
thread name PoolThread-twisted.internet.reactor-0 daemon False
*HANG*

Apparently the twisted reactor created by vncdotool is marked as a background thread but it spawns workers which are not marked as background and the program does not stop. Perhaps this used to work but was broken by updates to twisted itself? I know nothing about the twisted framework.

My goals is to automate clicking through VNC inside pytest and I found that this workaround works for me:

def pytest_sessionfinish():
    from vncdotool import api

    api.shutdown()

One idea to work around the reactor not shutting down automatically would be to reference-count it from connect/disconnect.

pmhahn commented 2 months ago

The Twisted reactor is not restartable! A reactor is only started by the API when no-one is yet running. Stopping it after with vncdotool.api.connect(…): … would prevent you from using the API again in the same process. Therefore vncdotool decided to keep the reactor running in a background thread, which is started on first use and needs to be shutdown explicitly by running shutdown() explicitly as you found out. That is why the function is there.

FYI: Twisted is async from a time when Python did not have nativ async itself, it sometimes matches badly with Threads and leads to all kind of problems when combined. Read Using Threads in Twisted and Choosing a Reactor. Even running it in a background threads leads to all kind of strange issues, e.g. signal handling not working correctly.

pevogam commented 2 weeks ago

The problem in my understanding is whether this should be considered bug or expected behavior. Personally I would consider it a bug since a lot of previous behavior is broken and we have circumstances where it is very hard to pin down where exactly to "shutdown" but perhaps I might have misunderstood something here and this is a new behavior we are expected to adapt to.

pevogam commented 2 weeks ago

And since I don't have the authority to mark duplicate issues I consider this related to https://github.com/sibson/vncdotool/issues/255.