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. Detailsimport 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 :) |
harshil21
left a comment
There was a problem hiding this comment.
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>
harshil21
left a comment
There was a problem hiding this comment.
looks good from my side. Another review would help - @Poolitzer do you want to review too since you created the issue?
| raise RuntimeError("This Application is not running!") | ||
|
|
||
| self._running = False | ||
| self.__stop_running_marker.clear() |
There was a problem hiding this comment.
why do we need this again here
There was a problem hiding this comment.
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:
Details