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
Conversation
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 Also removed the supression of 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() |
Latest commit also addresses #4156, at least party. By proply closing the |
Last commits allows calling |
SystemExit
raised in HandlersSystemExit
raised in Handlers
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 … |
After removing all other tests, |
Okay, for some reason now the tests run as well. I have no clue why … Anyway, I'm looking forward to reviews :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty happy with this PR. Though the shutdown logic has gotten complicated over the past few updates (at least to me), I did manually test various cases and it worked as I expected. Tests also seem to have been simplified (no threading?!)
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good from my side. Another review would help - @Poolitzer do you want to review too since you created the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
@@ -670,14 +672,26 @@ async def stop(self) -> None: | |||
raise RuntimeError("This Application is not running!") | |||
|
|||
self._running = False | |||
self.__stop_running_marker.clear() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this again here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case the app is stopped & restartet during the lifetime of the python script, we need to reset the marker. otherwise the second startup will immediately shut down as well
Co-authored-by: Poolitzer <github@poolitzer.eu>
closes #4156. closes #4155.
Have not checked with jobs or error handlers yet.MWE for checking the behavior: