Skip to content
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

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

Open
cdleonard opened this issue Jun 14, 2024 · 3 comments
Labels

Comments

@cdleonard
Copy link

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
Copy link
Collaborator

pmhahn commented Jun 16, 2024

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.

@pmhahn pmhahn added the bug label Jun 16, 2024
pmhahn added a commit to univention/vncdotool that referenced this issue Jun 17, 2024
sibson pushed a commit that referenced this issue Jun 22, 2024
* test: keys are released in reverse order

Since: a4bb988

* ci: Add Python 3.12 as supported

* ci: Quote shell variables

* ci: Move vncdo from /tmp/ to $PWD/.vncdo

Hard-coded paths below /tmp/ are a security issue on shared hosts.

/tmp/ is (often) cleaned up on reboot.

* doc: Fix building documentation

* doc: Improve documentation

Fix spelling errors.

Fix broken links, e.g. https:// everywhere.

Fix Sphinx reST markup.

* doc: Document API shutdown

Issue #288

* refactor: modernize type annotations

pyupgrade --py38-plus
ruff check --fix setup.py vncdotool/*.py

* fix: mouseDrag diagonally

move mouse diagonally instead of first going up/down and then
left/right.

Do not use `time.sleep()` with Twisted reactor as it will also pause all
other event processing.

Patch `vncev` to print all mouse events, including those where not button
is pressed / released.

Issue #287

* fixup! ci: Add Python 3.12 as supported

* fixup! doc: Improve documentation
@pevogam
Copy link
Contributor

pevogam commented Aug 23, 2024

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
Copy link
Contributor

pevogam commented Aug 23, 2024

And since I don't have the authority to mark duplicate issues I consider this related to #255.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants