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

Handle SystemExit raised in Handlers #4157

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Mar 12, 2024

closes #4156. closes #4155.
Have not checked with jobs or error handlers yet.

MWE for checking the behavior:

import asyncio
import logging
import sys

from telegram.ext import ApplicationBuilder, ContextTypes, TypeHandler

# Enable logging
logging.basicConfig(
    format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", level=logging.INFO
)
logging.getLogger("telegram.ext.Application").setLevel(logging.DEBUG)

async def raise_system_exit(_, context: ContextTypes.DEFAULT_TYPE):
    for ___ in range(5):
        await context.application.update_queue.put("Hello, world!")
    print("raising system exit")
    raise sys.exit(1)


async def post_init(application):
    async def put(app):
        await asyncio.sleep(2)
        print("enqueueing update")
        await app.update_queue.put("Hello, world!")

    application.create_task(put(application))


application = ApplicationBuilder().token("TOKEN").post_init(post_init).build()
application.add_handler(TypeHandler(object, raise_system_exit))

if __name__ == "__main__":
    application.run_polling()

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Mar 29, 2024

Did some more rework and also some testing. Seems to work with handler/error handler callbacks and jobs now. I also realized that when using block=False we don't have an issue in the first place: If the SystemExit is raised in a task that apparently is reported directory to the event loop.

Also removed the supression of CancelledError as already hinted in #4172

Exetnded example for fiddling around, including both a run_polling and a custom version.

import asyncio
import logging
import sys

from telegram.ext import ApplicationBuilder, ContextTypes, TypeHandler, Application

# Enable logging
logging.basicConfig(
    format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", level=logging.INFO
)
logging.getLogger("telegram.ext.Application").setLevel(logging.DEBUG)
logging.getLogger("telegram.ext.Updater").setLevel(logging.DEBUG)
logging.getLogger("httpx").setLevel(logging.WARNING)
logging.getLogger("httpcore").setLevel(logging.WARNING)


async def queue(application: Application):
    for ___ in range(5):
        await application.update_queue.put("Hello, world!")


async def queue_and_raise(context: ContextTypes.DEFAULT_TYPE):
    await queue(context.application)
    print("raising system exit")
    raise sys.exit(1)


async def raise_system_exit(_, context: ContextTypes.DEFAULT_TYPE):
    # await queue_and_raise(context)
    raise RuntimeError("This is a test error")


async def error_handler(_, context: ContextTypes.DEFAULT_TYPE):
    print("error handler got called")
    await queue_and_raise(context)


async def general_handler(update: object, context: ContextTypes.DEFAULT_TYPE):
    # await asyncio.sleep(1)
    # context.application.stop_running()
    print("general handler got called for update", update)


async def job_callback(context: ContextTypes.DEFAULT_TYPE):
    await queue_and_raise(context)


async def post_init(application):
    async def put(app):
        await asyncio.sleep(2)
        print("enqueueing update")
        await app.update_queue.put(-1)

    # application.create_task(put(application))
    application.job_queue.run_once(job_callback, 5)


application = (
    ApplicationBuilder().token("TOKEN")
    .post_init(post_init)
    .build()
)
application.add_handler(TypeHandler(int, raise_system_exit, block=True))
application.add_handler(TypeHandler(object, general_handler))
application.add_error_handler(error_handler, block=False)


def main():
    application.run_polling()


async def main_async():
    await application.initialize()
    await application.start()
    await application.updater.start_polling()
    await application.post_init(application)
    await queue(application)
    try:
        await asyncio.Event().wait()
    except Exception as ex:
        print(f"exception while running server: {ex!r}")
    except BaseException as ex:
        print(f"base exception while running server: {ex!r}")
        raise ex
    finally:
        print("stopping application")
        await application.stop()
        await application.updater.stop()
        await application.shutdown()


if __name__ == "__main__":
    # asyncio.run(main_async())
    main()

@Bibo-Joshi
Copy link
Member Author

Latest commit also addresses #4156, at least party. By proply closing the updater_coroutine, one can now raise SystemExit in post_init and still get a clean shutdown. I'll still think about if we can make stop_running available in a clean fashing in post_init as well.

@Bibo-Joshi
Copy link
Member Author

Last commits allows calling stop_running in post_init.
I still have to add tests for raising SystemExit in the different callbacks (will have to do so on a linux machine), but the code changes could already be reviewed

@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review April 7, 2024 16:30
@Bibo-Joshi Bibo-Joshi changed the title [PoC] Handle SystemExit raised in Handlers Handle SystemExit raised in Handlers Apr 15, 2024
@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Apr 17, 2024

https://github.com/python-telegram-bot/python-telegram-bot/actions/runs/8723054036/attempts/3#summary-23932096602

this test failure happens only on windows with python 3.11/3.12 and only that one single test. I don't get why sniffio isn't able to detect the asyncio lib in that particluar case and I'm unable to reproduce the problem locally (updated all transitive dependencies & python to match the setup in GitHub actions).

The failer doesn't look related to the PR to me, but it's still strange …

@Bibo-Joshi Bibo-Joshi mentioned this pull request Apr 26, 2024
@Bibo-Joshi
Copy link
Member Author

After removing all other tests, test_signal_handler failed only on py11 + windows and that succeeded after rerun … let's try adding rest of the tests again

@Bibo-Joshi
Copy link
Member Author

Okay, for some reason now the tests run as well. I have no clue why … Anyway, I'm looking forward to reviews :)

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

Successfully merging this pull request may close these issues.

[Bug] no clean way to exit application in post_init [Bug] sys.exit hangs in callback
1 participant