Repository: attzonko/mmpy_bot (language: python)

Issue:
Safely Exit
**Describe the bug**
Sorry if this isn't a bug, or if this is in the documentation, I couldn't find it and I can't find a community for mmpy_bot other than the github repo. How can I safely exit mmpy_bot in code? A couple use cases are:
1) a ci-cd setup where someone commits to a devops bot plugin, and the pipeline sends a shutdown signal to the bot
2) someone manually wants the bot to exit for various reasons (e.g. "@devops-bot exit")

There's a jobs command I poked around at earlier to see if there were any long running processes (let's say an action or deploy that takes 20-60 minutes) and you wouldn't want it to exit if they were still running, but I can iterate on that if I knew how to exit safely. 

Also, I apologize for being a beginner at python, if this is an obvious thing I am missing (sys.exit() doesn't work for me). 

**How To Reproduce**
sys.exit() doesn't exit the overarching Bot.run()

**Expected behavior**
Something to call when shutdown is needed to either restart things or an error has occurred that needs to be fatal. 

**Operating Environment (please complete the following information):**
 - OS: Ubuntu 20.04
 - Python Version: 3.8
 - Mattermost Version: 5.31.1 in gitlab
 - mmpy_bot Version: 1.3.9
 



Discussion / hints:
We have exactly this same setup. CI + `self-check-for-updates` + admin `shutdown`.

In 1.x I'm using:
```
def shutdown():
    """Kill the main process with a terminate signal"""
    os.kill(os.getpid(), signal.SIGTERM)
```

Not exactly graceful but gets the job done. All of the code we have is safe to be interrupted in this fashion.

As for why `sys.exit()` doesn't work, this is due to 1.x running with [threads that are allowed to persist after the main thread](https://github.com/attzonko/mmpy_bot/blob/v1.3.4/mmpy_bot/bot.py#L39).

@jneeven may correct me if I'm wrong but this will likely become a non-issue with the new mmpy_bot 2.0 version.
> @jneeven may correct me if I'm wrong but this will likely become a non-issue with the new mmpy_bot 2.0 version.

Exiting the main process will close any threads that aren't currently doing any work, but if they're in the middle of executing something, they will finish that first. If you do want to be able to close long running threads, you can periodically check for [`ThreadPool.alive`](https://github.com/attzonko/mmpy_bot/blob/master/mmpy_bot/threadpool.py#L20). The entire bot is currently built around threads and asyncio, but everything is setup in such a way that it should be easy to replace the `ThreadPool` with a custom `ProcessPool` if you need it (you'd just have to implement checking for `self.alive` in a different way, since it won't be shared across processes).
Updating the discussion to the newer 2.0 release. There are now a few stop mechanisms in place but I'm still unsure how to best trigger them. The solution I pasted above for 1.x **should not** be used as it will cause any [`Plugin.on_stop()`](https://github.com/attzonko/mmpy_bot/blob/d9c0d2282b9c3dd64d0a89dc0c3eda47eb73da2d/mmpy_bot/plugins/base.py#L68) to **not execute**.

It seems like sending a `signal.SIGINT` instead of a `signal.SIGTERM` [might work](https://github.com/attzonko/mmpy_bot/blob/4397f978e3cbd6912435a526ae4a3d58774975b6/mmpy_bot/bot.py#L90) but It feels a bit brute-force.

Since we have `Plugin.on_stop()` now is seems fitting that we also have an official **shutdown** mechanism documented and available in the core, ideally reachable from a `Plugin`. There's [`threadpool.stop()`](https://github.com/attzonko/mmpy_bot/blob/30a31a60dfc695f7eebe93341e4a97c8b684ba65/mmpy_bot/threadpool.py#L39) but since we also block on the [`websocket connection`](https://github.com/attzonko/mmpy_bot/blob/9691b67257e3105115e48d85148ce9d55c96e517/mmpy_bot/event_handler.py#L43) I'm not sure if stopping the threadpool is sufficient (not tested yet).

@jneeven any suggestions?
Right, I only now see what you mean... So the reason the bot keeps running until you send it an interrupt of some kind is the [event handler](https://www.github.com/attzonko/mmpy_bot/tree/master/mmpy_bot%2Fevent_handler.py). It basically starts an infinite loop that keeps checking for messages, and is therefore the only part of the code that doesn't respect anything other than a hard-exit.

There are three things we can do:
1. Allow the functions to return some kind of stop signal and start checking for those signals in the event handler when the functions are being dispatched. It can then call `exit()` when such a signal is detected.
2. Modify the driver code so that `connect_websocket` doesn't just loop forever, but respects some kind of `alive` variable just like the threadpool does. I'm not sure how ugly this would be; there could even already exist such a variable that we just need to identify.
3. We can add another asyncio loop specifically for this purpose, which just checks if the bot should stop running and shuts down if this is the case.

My preference would be 2 > 1 > 3. Off the top of my head I can't guarantee that number 1 is actually possible, and solution 3 is ugly. I think looping forever with no kind of check whatsoever is bad practice in general, so if `mattermost-driver` doesn't currently support a clean shutdown we should just make a PR. I would hope this would be fairly simple to implement.


For 2. the event handler already has access to `self.driver.threadpool.alive` so that could be easy if we are ok with crossing objects in this fashion.

As to breaking out of the loop, I think [this is the blocking code](https://github.com/Vaelor/python-mattermost-driver/blob/cb675a9f116b7c7ea30de302dd948b03c340031d/src/mattermostdriver/websocket.py#L52) and there's a timeout option that is inherited from the Driver (default 30 secs). **Edit**: which is meant to control the server heartbeat so not useful in our case. 
~~But if I understand it correctly, it looks like we could raise a `GracefulShutdown` exception in the [event handler function](https://github.com/attzonko/mmpy_bot/blob/9691b67257e3105115e48d85148ce9d55c96e517/mmpy_bot/event_handler.py#L65) that would break out of that loop and could be caught [here](https://github.com/attzonko/mmpy_bot/blob/9691b67257e3105115e48d85148ce9d55c96e517/mmpy_bot/event_handler.py#L44).~~

Actually, since this entire code is async, it feels like it should be possible to simply cancel the task but I haven't found the right location to do it yet.

Fix patch:
diff --git a/mmpy_bot/bot.py b/mmpy_bot/bot.py
index 7f3d8eb5..d09fde0f 100644
--- a/mmpy_bot/bot.py
+++ b/mmpy_bot/bot.py
@@ -53,6 +53,8 @@ def __init__(
         if self.settings.WEBHOOK_HOST_ENABLED:
             self._initialize_webhook_server()
 
+        self.running = False
+
     def _initialize_plugins(self, plugins: Sequence[Plugin]):
         for plugin in plugins:
             plugin.initialize(self.driver, self.settings)
@@ -72,6 +74,8 @@ def _initialize_webhook_server(self):
     def run(self):
         logging.info(f"Starting bot {self.__class__.__name__}.")
         try:
+            self.running = True
+
             self.driver.threadpool.start()
             # Start a thread to run potential scheduled jobs
             self.driver.threadpool.start_scheduler_thread(
@@ -88,13 +92,21 @@ def run(self):
             self.event_handler.start()
 
         except KeyboardInterrupt as e:
-            self.stop()
             raise e
 
+        finally:
+            # When either the event handler finishes (if we asked it to stop) or we
+            # receive a KeyboardInterrupt, shut down the bot.
+            self.stop()
+
     def stop(self):
+        if not self.running:
+            return
+
         logging.info("Stopping bot.")
         # Shutdown the running plugins
         for plugin in self.plugins:
             plugin.on_stop()
         # Stop the threadpool
         self.driver.threadpool.stop()
+        self.running = False
diff --git a/mmpy_bot/webhook_server.py b/mmpy_bot/webhook_server.py
index 3e88b204..6963b54a 100644
--- a/mmpy_bot/webhook_server.py
+++ b/mmpy_bot/webhook_server.py
@@ -63,7 +63,7 @@ async def start(self):
         asyncio.get_event_loop().create_task(self._obtain_responses_loop())
 
     async def stop(self):
-        self.app_runner.cleanup()
+        await self.app_runner.cleanup()
         self.running = False
 
     async def _obtain_responses_loop(self):


Test patch:
diff --git a/tests/unit_tests/bot_test.py b/tests/unit_tests/bot_test.py
index b5cfe523..7d6ecb71 100644
--- a/tests/unit_tests/bot_test.py
+++ b/tests/unit_tests/bot_test.py
@@ -36,16 +36,16 @@ def test_init(self, login):
         for plugin in plugins:
             assert plugin.initialize.called_once_with(bot.driver)
 
-    @mock.patch("mmpy_bot.driver.Driver.init_websocket")
     @mock.patch.multiple("mmpy_bot.Plugin", on_start=mock.DEFAULT, on_stop=mock.DEFAULT)
-    def test_run(self, init_websocket, bot, **mocks):
-        bot.run()
-        init_websocket.assert_called_once()
+    def test_run(self, bot, **mocks):
+        with mock.patch.object(bot.driver, "init_websocket") as init_websocket:
+            bot.run()
+            init_websocket.assert_called_once()
 
-        for plugin in bot.plugins:
-            plugin.on_start.assert_called_once()
+            for plugin in bot.plugins:
+                plugin.on_start.assert_called_once()
 
-        bot.stop()
+            bot.stop()
 
-        for plugin in bot.plugins:
-            plugin.on_stop.assert_called_once()
+            for plugin in bot.plugins:
+                plugin.on_stop.assert_called_once()