[PATCH v1 0/2] Automatic recovery from a MySQL restart

[PATCH v1 0/2] Automatic recovery from a MySQL restart

[Ammar Faizi]

Hi,

This series adds a mechanism for the daemon to recover from a MySQL
restart event. Currently, the daemon is totally unusable when the MySQL
server is restarted. It's spinning in a loop with the following error:

   mysql.connector.errors.OperationalError: 2013 (HY000): Lost connection to MySQL server during query

When it happens, the only way to fix the situation is: restart the
daemon manually, which is obviously not productive. Create a mechanism
in the class DB to allow the caller to reconnect. This way, the caller
can automatically reconnect without having the user restart the daemon.

Handle MySQL error in the main daemon loop path. Do reconnect to the
database if such an error happens. This way, the daemon can
automatically recover from the MySQL server restart without having the
user restart the daemon.

This series is just a hot fix, there are two patches in this series:

  - Patch 1 is a preparation patch to create a recover mechanism.
  - Patch 2 is to implement the recover mechanism in the main daemon
    loop path.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---

Ammar Faizi (2):
  daemon: telegram: db: Allow the caller to reconnect
  daemon: telegram: Handle MySQL error

 daemon/telegram/database/core.py   | 31 +++++++++++++++++++++++++-----
 daemon/telegram/mailer/listener.py | 29 ++++++++++++++++++++++++----
 daemon/telegram/packages/client.py |  8 ++++----
 daemon/tg.py                       |  4 ++--
 4 files changed, 57 insertions(+), 15 deletions(-)


base-commit: 6e94cf607287e5bc209e9c14c8156c7ad49455e3
-- 
Ammar Faizi

[PATCH v1 1/2] daemon: telegram: db: Allow the caller to reconnect

[Ammar Faizi]

The daemon is totally unusable when the MySQL server is restarted. It's
spinning in a loop with the following error:

   mysql.connector.errors.OperationalError: 2013 (HY000): Lost connection to MySQL server during query

When it happens, the only way to fix the situation is: restart the
daemon manually, which is obviously not productive. Create a mechanism
in the class DB to allow the caller to reconnect. This way, the caller
can automatically reconnect without having the user restart the daemon.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 daemon/telegram/database/core.py   | 31 +++++++++++++++++++++++++-----
 daemon/telegram/packages/client.py |  8 ++++----
 daemon/tg.py                       |  4 ++--
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/daemon/telegram/database/core.py b/daemon/telegram/database/core.py
index c34d7a8..bc65b54 100644
--- a/daemon/telegram/database/core.py
+++ b/daemon/telegram/database/core.py
@@ -4,17 +4,38 @@
 # Copyright (C) 2022  Ammar Faizi <ammarfaizi2@gnuweeb.org>
 #
 
-
+from mysql import connector
 from .methods import DBMethods
 
 
 class DB(DBMethods):
-	def __init__(self, conn):
-		self.conn = conn
+	def __init__(self, host, user, password, database):
+		self.host = host
+		self.user = user
+		self.password = password
+		self.database = database
+		self.conn = None
+		self.connect()
+
+	#
+	# Allow the caller to reconnect.
+	#
+	def connect(self):
+		if self.conn:
+			self.close()
+
+		self.conn = connector.connect(
+			host=self.host,
+			user=self.user,
+			password=self.password,
+			database=self.database
+		)
 		self.conn.autocommit = True
 		self.cur = self.conn.cursor(buffered=True)
 
-
-	def __del__(self):
+	def close(self):
 		self.cur.close()
 		self.conn.close()
+
+	def __del__(self):
+		self.close()
diff --git a/daemon/telegram/packages/client.py b/daemon/telegram/packages/client.py
index c971ea1..2ee37f6 100644
--- a/daemon/telegram/packages/client.py
+++ b/daemon/telegram/packages/client.py
@@ -15,11 +15,11 @@ from .decorator import handle_flood
 
 
 class DaemonClient(Client):
-	def __init__(self, name: str, api_id: int,
-		api_hash: str, conn, **kwargs):
+	def __init__(self, name: str, api_id: int, api_hash: str, db: DB,
+		     **kwargs):
 		super().__init__(name, api_id,
-				api_hash, **kwargs)
-		self.db = DB(conn)
+				 api_hash, **kwargs)
+		self.db = db
 
 
 	@handle_flood
diff --git a/daemon/tg.py b/daemon/tg.py
index 5f8c21e..e6d5d05 100644
--- a/daemon/tg.py
+++ b/daemon/tg.py
@@ -6,11 +6,11 @@
 
 from apscheduler.schedulers.asyncio import AsyncIOScheduler
 from dotenv import load_dotenv
-from mysql import connector
 from atom import Scraper
 from telegram.packages import DaemonClient
 from telegram.mailer import BotMutexes
 from telegram.mailer import Bot
+from telegram.database import DB
 import os
 
 
@@ -22,7 +22,7 @@ def main():
 		api_id=int(os.getenv("API_ID")),
 		api_hash=os.getenv("API_HASH"),
 		bot_token=os.getenv("BOT_TOKEN"),
-		conn=connector.connect(
+		db=DB(
 			host=os.getenv("DB_HOST"),
 			user=os.getenv("DB_USER"),
 			password=os.getenv("DB_PASS"),
-- 
Ammar Faizi

[PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

A previous patch creates a mechanism to allow the caller to reconnect
to the database by calling db.connect(). Handle MySQL error in the
main daemon loop path. Do reconnect to the database if such an error
happens. This way, the daemon can automatically recover from the MySQL
server restart without having the user restart the daemon.

Signed-off-by: Ammar Faizi <ammarfaizi2@gnuweeb.org>
---
 daemon/telegram/mailer/listener.py | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/daemon/telegram/mailer/listener.py b/daemon/telegram/mailer/listener.py
index 1c92f23..8a61367 100644
--- a/daemon/telegram/mailer/listener.py
+++ b/daemon/telegram/mailer/listener.py
@@ -7,6 +7,7 @@
 from pyrogram.types import Message
 from apscheduler.schedulers.asyncio import AsyncIOScheduler
 from telegram.packages import DaemonClient
+from mysql import connector
 from atom import Scraper
 from atom import utils
 from enums import Platform
@@ -15,6 +16,7 @@ import re
 import traceback
 
 
+
 class BotMutexes():
 	def __init__(self):
 		self.send_to_tg = asyncio.Lock()
@@ -43,13 +45,32 @@ class Bot():
 		)
 
 
+	async def handle_db_error(self, e):
+		#
+		# TODO(ammarfaizi2):
+		# Ideally, we also want to log and report this situation.
+		#
+		print(f"Database error: {str(e)}")
+		print("Reconnecting to the database...")
+		self.db.connect()
+
+		#
+		# Don't do this too often, avoid connect() burst.
+		#
+		await asyncio.sleep(3)
+
+
 	async def __run(self):
 		print("[__run]: Running...")
-		for url in self.db.get_atom_urls():
-			try:
+		try:
+			for url in self.db.get_atom_urls():
 				await self.__handle_atom_url(url)
-			except:
-				print(traceback.format_exc())
+		except connector.errors.OperationalError as e:
+			await self.handle_db_error(e)
+		except connector.errors.DatabaseError as e:
+			await self.handle_db_error(e)
+		except:
+			print(traceback.format_exc())
 
 		if not self.isRunnerFixed:
 			self.isRunnerFixed = True
-- 
Ammar Faizi

Re: [PATCH v1 1/2] daemon: telegram: db: Allow the caller to reconnect

[Muhammad Rizki]

On 27/10/2022 22.08, Ammar Faizi wrote:
> +	def connect(self):
> +		if self.conn:
> +			self.close()
> +
> +		self.conn = connector.connect(

Have you tried the ping() method?

Refer: 
http://www.neotitans.com/resources/python/mysql-python-connection-error-2006.html

The ping()'s doc says:
     Checks whether or not the connection to the server is
     working. If it has gone down, an automatic reconnection is
     attempted.

     This function can be used by clients that remain idle for a
     long while, to check whether or not the server has closed the
     connection and reconnect if necessary.

I think this is more practice. I haven't test the ping() method but, you 
got the idea? Thanks.

Re: [PATCH v1 1/2] daemon: telegram: db: Allow the caller to reconnect

[Muhammad Rizki]

On 27/10/2022 23.52, Muhammad Rizki wrote:
> On 27/10/2022 22.08, Ammar Faizi wrote:
>> +    def connect(self):
>> +        if self.conn:
>> +            self.close()
>> +
>> +        self.conn = connector.connect(
> 
> Have you tried the ping() method?
> 
> Refer: 
> http://www.neotitans.com/resources/python/mysql-python-connection-error-2006.html

Sorry, I mean this: 
https://dev.mysql.com/doc/connector-python/en/connector-python-api-mysqlconnection-ping.html

Re: [PATCH v1 1/2] daemon: telegram: db: Allow the caller to reconnect

[Ammar Faizi]

On 10/27/22 11:54 PM, Muhammad Rizki wrote:
> On 27/10/2022 23.52, Muhammad Rizki wrote:
>> On 27/10/2022 22.08, Ammar Faizi wrote:
>>> +    def connect(self):
>>> +        if self.conn:
>>> +            self.close()
>>> +
>>> +        self.conn = connector.connect(
>>
>> Have you tried the ping() method?
>>
>> Refer: http://www.neotitans.com/resources/python/mysql-python-connection-error-2006.html
> 
> Sorry, I mean this: https://dev.mysql.com/doc/connector-python/en/connector-python-api-mysqlconnection-ping.html

Thanks for the great feedback!

Yes, using ping() can perform the reconnect. This way we don't have to
save the connection credentials in DB class. I'll send a v2 later with
the ping() method applied.

Just a fast test on top of this series to see whether ping really works:

diff --git a/daemon/telegram/database/core.py b/daemon/telegram/database/core.py
index bc65b54..6260c5b 100644
--- a/daemon/telegram/database/core.py
+++ b/daemon/telegram/database/core.py
@@ -22,7 +22,8 @@ class DB(DBMethods):
  	#
  	def connect(self):
  		if self.conn:
-			self.close()
+			self.conn.ping(reconnect=True, attempts=3, delay=3)
+			return
  
  		self.conn = connector.connect(
  			host=self.host,


-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Thu, Oct 27, 2022 at 10:08 PM Ammar Faizi wrote:
> +       async def handle_db_error(self, e):
> +               #
> +               # TODO(ammarfaizi2):
> +               # Ideally, we also want to log and report this situation.
> +               #
> +               print(f"Database error: {str(e)}")
> +               print("Reconnecting to the database...")
> +               self.db.connect()
> +
> +               #
> +               # Don't do this too often, avoid connect() burst.
> +               #
> +               await asyncio.sleep(3)

What about sleeping first before connecting?
As it'll give a chance for the server to start properly, MySQL server
may take several seconds to restart.

-- Viro

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Thu, Oct 27, 2022 at 10:08 PM Ammar Faizi wrote:
>         async def __run(self):
>                 print("[__run]: Running...")
> -               for url in self.db.get_atom_urls():
> -                       try:
> +               try:
> +                       for url in self.db.get_atom_urls():
>                                 await self.__handle_atom_url(url)
> -                       except:
> -                               print(traceback.format_exc())
> +               except connector.errors.OperationalError as e:
> +                       await self.handle_db_error(e)
> +               except connector.errors.DatabaseError as e:
> +                       await self.handle_db_error(e)
> +               except:
> +                       print(traceback.format_exc())

I think this reconnect() handling should be done in each DB method,
not here, otherwise we'll potentially have a half-inserted data and
may get it wrong.

-- Viro

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/28/22 4:21 PM, Alviro Iskandar Setiawan wrote:
> On Thu, Oct 27, 2022 at 10:08 PM Ammar Faizi wrote:
>> +       async def handle_db_error(self, e):
>> +               #
>> +               # TODO(ammarfaizi2):
>> +               # Ideally, we also want to log and report this situation.
>> +               #
>> +               print(f"Database error: {str(e)}")
>> +               print("Reconnecting to the database...")
>> +               self.db.connect()
>> +
>> +               #
>> +               # Don't do this too often, avoid connect() burst.
>> +               #
>> +               await asyncio.sleep(3)
> 
> What about sleeping first before connecting?
> As it'll give a chance for the server to start properly, MySQL server
> may take several seconds to restart.

ping(reconnect=True, attempts=3, delay=3) should handle this better.
I'll send a v2 later.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/28/22 4:24 PM, Alviro Iskandar Setiawan wrote:
> I think this reconnect() handling should be done in each DB method,
> not here, otherwise we'll potentially have a half-inserted data and
> may get it wrong.

By using a transaction, such a situation cannot happen. It already
fulfills the ACID requirements. But I doubt we've fully handled that
properly. This needs extra code review, especially in that critical
section.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Fri, Oct 28, 2022 at 4:32 PM Ammar Faizi wrote:
> On 10/28/22 4:24 PM, Alviro Iskandar Setiawan wrote:
>> I think this reconnect() handling should be done in each DB method,
>> not here, otherwise we'll potentially have a half-inserted data and
>> may get it wrong.
>
> By using a transaction, such a situation cannot happen. It already
> fulfills the ACID requirements. But I doubt we've fully handled that
> properly. This needs extra code review, especially in that critical
> section.

Turning on autocommit for everything is usually a hint that you did
ACID wrong, maybe not you, but Rizki.

-- Viro

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/28/22 4:40 PM, Alviro Iskandar Setiawan wrote:
> On Fri, Oct 28, 2022 at 4:32 PM Ammar Faizi wrote:
>> On 10/28/22 4:24 PM, Alviro Iskandar Setiawan wrote:
>>> I think this reconnect() handling should be done in each DB method,
>>> not here, otherwise we'll potentially have a half-inserted data and
>>> may get it wrong.
>>
>> By using a transaction, such a situation cannot happen. It already
>> fulfills the ACID requirements. But I doubt we've fully handled that
>> properly. This needs extra code review, especially in that critical
>> section.
> 
> Turning on autocommit for everything is usually a hint that you did
> ACID wrong, maybe not you, but Rizki.

Maybe, that's why I said "I doubt". But I don't have time to review it
for now, will do that later.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/28/22 4:43 PM, Ammar Faizi wrote:
> On 10/28/22 4:40 PM, Alviro Iskandar Setiawan wrote:
>> On Fri, Oct 28, 2022 at 4:32 PM Ammar Faizi wrote:
>>> On 10/28/22 4:24 PM, Alviro Iskandar Setiawan wrote:
>>>> I think this reconnect() handling should be done in each DB method,
>>>> not here, otherwise we'll potentially have a half-inserted data and
>>>> may get it wrong.
>>>
>>> By using a transaction, such a situation cannot happen. It already
>>> fulfills the ACID requirements. But I doubt we've fully handled that
>>> properly. This needs extra code review, especially in that critical
>>> section.
>>
>> Turning on autocommit for everything is usually a hint that you did
>> ACID wrong, maybe not you, but Rizki.
> 
> Maybe, that's why I said "I doubt". But I don't have time to review it
> for now, will do that later.

I've just checked it. And yes, we have haven't fulfilled the ACID
requirements here. While the following __send_mail method is holding the
mutex while it's operating, it doesn't perform a rollback() operation if
the subsequent SQL queries fail.

So yes, having autocommit = True for everything is absolutely the wrong
thing to do. We have to fix this issue at some point.

It's currently working fine, only the corner case, that's when the MySQL
hits error in the middle of __send_mail function.

I believe there are more issues like this somewhere else. But this is
the most hot path to get spotted. We also have to teach Rizki about this
so he can fix it himself.

# @__must_hold(self.mutexes.send_to_tg)
async def __send_mail(self, url, mail, tg_chat_id):
	email_msg_id = utils.get_email_msg_id(mail)
	if not email_msg_id:
		#
		# It doesn't have a Message-Id.
		# A malformed email. Skip!
		#
		return False

	email_id = self.__mail_id_from_db(email_msg_id,
						tg_chat_id)
	if not email_id:
		#
		# Email has already been sent to Telegram.
		# Skip!
		#
		return False

	text, files, is_patch = utils.create_template(mail, Platform.TELEGRAM)
	reply_to = self.get_reply(mail, tg_chat_id)
	url = str(re.sub(r"/raw$", "", url))

	if is_patch:
		m: "Message" = await self.client.send_patch_email(
			mail, tg_chat_id, text, reply_to, url
		)
	else:
		text = "#ml\n" + text
		m: "Message" = await self.client.send_text_email(
			tg_chat_id, text,reply_to, url
		)

	self.db.save_telegram_mail(email_id, m.chat.id, m.id)
	for d, f in files:
		await m.reply_document(f"{d}/{f}", file_name=f)
		await asyncio.sleep(1)

	utils.remove_patch(files)

	return True

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Fri, Oct 28, 2022 at 11:29 PM Ammar Faizi wrote:
> I believe there are more issues like this somewhere else. But this is
> the most hot path to get spotted. We also have to teach Rizki about this
> so he can fix it himself.

Yes.

>         if is_patch:
>                 m: "Message" = await self.client.send_patch_email(
>                         mail, tg_chat_id, text, reply_to, url
>                 )
>         else:
>                 text = "#ml\n" + text
>                 m: "Message" = await self.client.send_text_email(
>                         tg_chat_id, text,reply_to, url
>                 )
>
>         self.db.save_telegram_mail(email_id, m.chat.id, m.id)
>         for d, f in files:
>                 await m.reply_document(f"{d}/{f}", file_name=f)
>                 await asyncio.sleep(1)
>
>         utils.remove_patch(files)
>
>         return True

Even this part is also problematic, if you hit an error before
utils.remove_patch(), the patch file won't get removed because
exception will stop the current execution and throw the exception to
the previous call stack.

I hate try and catch style, because it's very easy to miss an error
like this. I prefer the way C handles error manually via return value
like:

        ret = func_call();
        if (ret < 0)
                goto clean_up;

But that's obviously not the Python style. Python prefers using
exceptions which make the caller may not be aware of this cleanup
strategy!

-- Viro

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/28/22 11:46 PM, Alviro Iskandar Setiawan wrote:
> Even this part is also problematic, if you hit an error before
> utils.remove_patch(), the patch file won't get removed because
> exception will stop the current execution and throw the exception to
> the previous call stack.
> 
> I hate try and catch style, because it's very easy to miss an error
> like this. I prefer the way C handles error manually via return value
> like:
> 
>          ret = func_call();
>          if (ret < 0)
>                  goto clean_up;
> 
> But that's obviously not the Python style. Python prefers using
> exceptions which make the caller may not be aware of this cleanup
> strategy!

Ditto.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 28/10/22 23.29, Ammar Faizi wrote:
> So yes, having autocommit = True for everything is absolutely the wrong

So, you want me to remove the autocommit = True and manual commit each 
we want to insert and delete?

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On Sat, Oct 29, 2022 at 12:02 AM Muhammad Rizki <kiizuha@gnuweeb.org> wrote:
> So, you want me to remove the autocommit = True and manual commit each
> we want to insert and delete?

Yes, but note that, not only manual commit, you have to make sure the
operation is atomic.

For example:
  begin_transaction()
  insert_a  -> this is a success
  insert_b  -> this is a success
  insert_c  -> this is a fail, goto rollback
  commit() -> this doesn't happen

rollback:
  rollback() // insert_a and insert_b are undone.

Those 3 inserts are atomic and consistent. If one fails, all
operations must also fail. No half-insert, no unisolated select, no
missing cleanup, no temp file not deleted due to error in the middle
operation, etc.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 28/10/22 23.46, Alviro Iskandar Setiawan wrote:
> Even this part is also problematic, if you hit an error before
> utils.remove_patch(), the patch file won't get removed because
> exception will stop the current execution and throw the exception to
> the previous call stack.

You are right. So we just check every value from the DB method and if 
it's None just return like goto?

> 
> I hate try and catch style, because it's very easy to miss an error
> like this. I prefer the way C handles error manually via return value
> like:
> 
>          ret = func_call();
>          if (ret < 0)
>                  goto clean_up;
> 

I've improved the remove_patch() earlier using the glob UNIX style path 
to check if all temp dirs is exists then remove them all. What do you think?

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 00.19, Ammar Faizi wrote:
>
> For example:
>    begin_transaction()
>    insert_a  -> this is a success
>    insert_b  -> this is a success
>    insert_c  -> this is a fail, goto rollback
>    commit() -> this doesn't happen
> 
> rollback:
>    rollback() // insert_a and insert_b are undone.

This pseudo-code seems like C style. Anyway, the rollback() is from the 
MySQL function?

> 
> Those 3 inserts are atomic and consistent. If one fails, all
> operations must also fail. No half-insert, no unisolated select, no
> missing cleanup, no temp file not deleted due to error in the middle
> operation, etc.
> 

This is a little bit confused me to implement it. 🤔

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 01.15, Muhammad Rizki wrote:
> On 29/10/22 00.19, Ammar Faizi wrote:
>>
>> For example:
>>    begin_transaction()
>>    insert_a  -> this is a success
>>    insert_b  -> this is a success
>>    insert_c  -> this is a fail, goto rollback
>>    commit() -> this doesn't happen
>>
>> rollback:
>>    rollback() // insert_a and insert_b are undone.
> 
> This pseudo-code seems like C style. Anyway, the rollback() is from the 
> MySQL function?
> 
>>
>> Those 3 inserts are atomic and consistent. If one fails, all
>> operations must also fail. No half-insert, no unisolated select, no
>> missing cleanup, no temp file not deleted due to error in the middle
>> operation, etc.
>>
> 
> This is a little bit confused me to implement it. 🤔

Anyway, I've ever wanted to refactor the DB using the SQLAlchemy and 
want to implement the clean-arch method too, but, IDK when to do that, 
maybe I could just re-create the daemon in my local and make the version 
2 of it.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Sat, Oct 29, 2022 at 1:10 AM Muhammad Rizki wrote:
> On 28/10/22 23.46, Alviro Iskandar Setiawan wrote:
>> Even this part is also problematic, if you hit an error before
>> utils.remove_patch(), the patch file won't get removed because
>> exception will stop the current execution and throw the exception to
>> the previous call stack.
>
> You are right. So we just check every value from the DB method and if
> it's None just return like goto?

You don't use goto in Python. In an OOP style, the cleanup usually
happens in the destructor, or in a "try with finally" statement.

>>
>> I hate try and catch style, because it's very easy to miss an error
>> like this. I prefer the way C handles error manually via return value
>> like:
>>
>>          ret = func_call();
>>          if (ret < 0)
>>                  goto clean_up;
>>
>
> I've improved the remove_patch() earlier using the glob UNIX style path
> to check if all temp dirs is exists then remove them all. What do you think?

It doesn't address the issue. You still don't understand the
underlying issue behind your remove_patch() placement.

You have this:

        for d, f in files:
                await m.reply_document(f"{d}/{f}", file_name=f)
                await asyncio.sleep(1)

        utils.remove_patch(files)

What happens if you follow that for loop, then m.reply_document()
throws an exception?

The answer is:
utils.remove_patch(files) will *not* be executed because it will throw
the exception to the previous call stack, and if the previous call
stack doesn't have a "try and except" statement, it will throw the
exception to the previous call stack again, until at some point it is
handled by and "try and except". If it never hits a "try and except"
statement, the application will terminate.

You don't do the cleanup if:
    - An error happens *after you create the patch file*.
    - But before the remove_patch() is executed.

There are 2 possible solutions, either using a "try with finally"
statement, or using a destructor wrapped in a class.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 01.26, Alviro Iskandar Setiawan wrote:
> On Sat, Oct 29, 2022 at 1:10 AM Muhammad Rizki wrote:
>> On 28/10/22 23.46, Alviro Iskandar Setiawan wrote:
>>> Even this part is also problematic, if you hit an error before
>>> utils.remove_patch(), the patch file won't get removed because
>>> exception will stop the current execution and throw the exception to
>>> the previous call stack.
>>
>> You are right. So we just check every value from the DB method and if
>> it's None just return like goto?
> 
> You don't use goto in Python. In an OOP style, the cleanup usually
> happens in the destructor, or in a "try with finally" statement.
> 
>>>
>>> I hate try and catch style, because it's very easy to miss an error
>>> like this. I prefer the way C handles error manually via return value
>>> like:
>>>
>>>           ret = func_call();
>>>           if (ret < 0)
>>>                   goto clean_up;
>>>
>>
>> I've improved the remove_patch() earlier using the glob UNIX style path
>> to check if all temp dirs is exists then remove them all. What do you think?
> 
> It doesn't address the issue. You still don't understand the
> underlying issue behind your remove_patch() placement.
> 
> You have this:
> 
>          for d, f in files:
>                  await m.reply_document(f"{d}/{f}", file_name=f)
>                  await asyncio.sleep(1)
> 
>          utils.remove_patch(files)
> 
> What happens if you follow that for loop, then m.reply_document()
> throws an exception?

It will throw an exception before executing the remove_patch(), but, the 
problem is not "if m.reply_document() throw an exception", but the files 
variable from the utils.create_template() is the problem if throw an 
exception, I think it will be throw a local variable error. Here is my 
improved remove_patch():


     def remove_patch(
	tmp: Union[str, list] = None,
	platform: Platform = Platform.TELEGRAM
     ):
	    if isinstance(tmp, str):
		    return shutil.rmtree(tmp)

             # check if the tmp is None or an empty list or empty str
             # if they are empty or None then remove using glob UNIX
             # style path which is if empty will not throw an error
	    if not bool(tmp):
		    platform = platform.TELEGRAM.value
		    store_dir = os.getenv("STORAGE_DIR", "storage")
		    for d in glob.glob(f"{platform}/{store_dir}/*/"):
			    shutil.rmtree(d)
		    return

	    for d,_ in tmp:
		    shutil.rmtree(d)


> There are 2 possible solutions, either using a "try with finally"
> statement, or using a destructor wrapped in a class.

I will do it if this PATCH series is done and will do it in logger PATCH 
series later. I wait for sir Ammar create the v2 of this series and pull 
it after this PATCH series is applied to the master branch.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Sat, Oct 29, 2022 at 1:52 AM Muhammad Rizki wrote:
> It will throw an exception before executing the remove_patch(), but, the
> problem is not "if m.reply_document() throw an exception", but the files
> variable from the utils.create_template() is the problem if throw an
> exception, I think it will be throw a local variable error.

"if m.reply_document() throws an exception" is a REAL PROBLEM.

  - m.reply_document() performs a network I/O. Therefore it may FAIL.
  - send_patch_email() performs a network I/O. Therefore it may FAIL.
  - send_text_email() performs a network I/O. Therefore it may FAIL.
  - MySQL queries perform a network I/O. Therefore they may FAIL.
  - whatever...

If one of them fails, remove_patch() *is not executed*.

> Here is my improved remove_patch():
>
>
>      def remove_patch(
>         tmp: Union[str, list] = None,
>         platform: Platform = Platform.TELEGRAM
>      ):
>             if isinstance(tmp, str):
>                     return shutil.rmtree(tmp)
>
>              # check if the tmp is None or an empty list or empty str
>              # if they are empty or None then remove using glob UNIX
>              # style path which is if empty will not throw an error
>             if not bool(tmp):
>                     platform = platform.TELEGRAM.value
>                     store_dir = os.getenv("STORAGE_DIR", "storage")
>                     for d in glob.glob(f"{platform}/{store_dir}/*/"):
>                             shutil.rmtree(d)
>                     return

This is insane. That listener is running asynchronously, and you
delete all temporary files without holding any mutex. At that time,
another async job *may still be using* the files you batch-delete in
this code.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.08, Alviro Iskandar Setiawan wrote:
>
>> Here is my improved remove_patch():
>>
>>
>>       def remove_patch(
>>          tmp: Union[str, list] = None,
>>          platform: Platform = Platform.TELEGRAM
>>       ):
>>              if isinstance(tmp, str):
>>                      return shutil.rmtree(tmp)
>>
>>               # check if the tmp is None or an empty list or empty str
>>               # if they are empty or None then remove using glob UNIX
>>               # style path which is if empty will not throw an error
>>              if not bool(tmp):
>>                      platform = platform.TELEGRAM.value
>>                      store_dir = os.getenv("STORAGE_DIR", "storage")
>>                      for d in glob.glob(f"{platform}/{store_dir}/*/"):
>>                              shutil.rmtree(d)
>>                      return
> 
> This is insane. That listener is running asynchronously, and you
> delete all temporary files without holding any mutex. At that time,
> another async job *may still be using* the files you batch-delete in
> this code.

Sorry, you mention remove_patch() which function from? Telegram bot 
listener __send_mail() ? if yes, that function is already handled by 
mutex, see it on telegram/mailer/listener.py of __handle_mail(), in line 
77 it is already hold.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 1:52 AM, Muhammad Rizki wrote:
>      def remove_patch(
>      tmp: Union[str, list] = None,
>      platform: Platform = Platform.TELEGRAM
>      ):
>          if isinstance(tmp, str):
>              return shutil.rmtree(tmp)
> 
>              # check if the tmp is None or an empty list or empty str

isinstance("", str) is True, so this won't be reached if you pass an
empty string. It'll early return at "if isinstance("", str)".

I think it's the caller responsibility not to pass an invalid file
name. If the caller passes an invalid @tmp value, then you code it
wrong.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.16, Ammar Faizi wrote:
> On 10/29/22 1:52 AM, Muhammad Rizki wrote:
>>      def remove_patch(
>>      tmp: Union[str, list] = None,
>>      platform: Platform = Platform.TELEGRAM
>>      ):
>>          if isinstance(tmp, str):
>>              return shutil.rmtree(tmp)
>>
>>              # check if the tmp is None or an empty list or empty str
> 
> isinstance("", str) is True, so this won't be reached if you pass an
> empty string. It'll early return at "if isinstance("", str)".
> 
> I think it's the caller responsibility not to pass an invalid file
> name. If the caller passes an invalid @tmp value, then you code it
> wrong.
> 

So, just swap the `if tmp is None` to the first place, then ` if 
isinstance()`?

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 2:19 AM, Muhammad Rizki wrote:
> So, just swap the `if tmp is None` to the first place, then ` if isinstance()`?

Why would anyone ever call remove_patch(None)?

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.19, Ammar Faizi wrote:
> On 10/29/22 2:19 AM, Muhammad Rizki wrote:
>> So, just swap the `if tmp is None` to the first place, then ` if 
>> isinstance()`?
> 
> Why would anyone ever call remove_patch(None)?
> 

I create that statement because:

```
	# @__must_hold(self.mutexes.send_to_tg)
	async def __send_mail(self, url, mail, tg_chat_id):
		email_msg_id = utils.get_email_msg_id(mail)
		if not email_msg_id:
			#
			# It doesn't have a Message-Id.
			# A malformed email. Skip!
			#
			return False

		email_id = self.__mail_id_from_db(email_msg_id,
							tg_chat_id)
		if not email_id:
			#
			# Email has already been sent to Telegram.
			# Skip!
			#
			return False

		try:
			text, files, is_patch = utils.create_template(mail, Platform.TELEGRAM)
		except:

			# ensure the created tmp dir is removed
			utils.remove_patch()

			exc_str = utils.catch_err()
			self.client.logger.warning(exc_str)
			await self.client.send_log_file(url)

			# return, to avoid local variable error
			return

		reply_to = self.get_reply(mail, tg_chat_id)
		url = str(re.sub(r"/raw$", "", url))

		if is_patch:
                         ...
```

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Sat, Oct 29, 2022 at 2:15 AM Muhammad Rizki <kiizuha@gnuweeb.org> wrote:
> Sorry, you mention remove_patch() which function from? Telegram bot
> listener __send_mail() ? if yes, that function is already handled by
> mutex, see it on telegram/mailer/listener.py of __handle_mail(), in line
> 77 it is already hold.

Is this function only called from one place?
If that's a guarantee, I'm fine with that. But why delete all files,
it's the caller's problem that it doesn't call the function properly.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 2:22 AM, Muhammad Rizki wrote:
> I create that statement because:
[...]
>          try:
>              text, files, is_patch = utils.create_template(mail, Platform.TELEGRAM)
>          except:
> 
>              # ensure the created tmp dir is removed
>              utils.remove_patch()
> 
>              exc_str = utils.catch_err()
>              self.client.logger.warning(exc_str)
>              await self.client.send_log_file(url)
> 
>              # return, to avoid local variable error
>              return

This is ugly, will you do the:

     exc_str = utils.catch_err()
     self.client.logger.warning(exc_str)
     await self.client.send_log_file(url)

all over the place when error may happen?

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 2:29 AM, Alviro Iskandar Setiawan wrote:
> On Sat, Oct 29, 2022 at 2:15 AM Muhammad Rizki <kiizuha@gnuweeb.org> wrote:
>> Sorry, you mention remove_patch() which function from? Telegram bot
>> listener __send_mail() ? if yes, that function is already handled by
>> mutex, see it on telegram/mailer/listener.py of __handle_mail(), in line
>> 77 it is already hold.
> 
> Is this function only called from one place?
> If that's a guarantee, I'm fine with that. But why delete all files,
> it's the caller's problem that it doesn't call the function properly.

It's also called from daemon/telegram/packages/client.py.

That place doesn't hold the mutex anyway.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 2:34 AM, Ammar Faizi wrote:
> On 10/29/22 2:29 AM, Alviro Iskandar Setiawan wrote:
>> On Sat, Oct 29, 2022 at 2:15 AM Muhammad Rizki <kiizuha@gnuweeb.org> wrote:
>>> Sorry, you mention remove_patch() which function from? Telegram bot
>>> listener __send_mail() ? if yes, that function is already handled by
>>> mutex, see it on telegram/mailer/listener.py of __handle_mail(), in line
>>> 77 it is already hold.
>>
>> Is this function only called from one place?
>> If that's a guarantee, I'm fine with that. But why delete all files,
>> it's the caller's problem that it doesn't call the function properly.
> 
> It's also called from daemon/telegram/packages/client.py.
> 
> That place doesn't hold the mutex anyway.

Correction, that place may hold or may not hold the mutex, depending
where they are called. Another place that calls the remove_patch() is

    daemon/telegram/packages/plugins/commands/scrape.py

This one always doesn't hold the mutex.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 2:36 AM, Ammar Faizi wrote:
> On 10/29/22 2:34 AM, Ammar Faizi wrote:
>> On 10/29/22 2:29 AM, Alviro Iskandar Setiawan wrote:
>>> On Sat, Oct 29, 2022 at 2:15 AM Muhammad Rizki <kiizuha@gnuweeb.org> wrote:
>>>> Sorry, you mention remove_patch() which function from? Telegram bot
>>>> listener __send_mail() ? if yes, that function is already handled by
>>>> mutex, see it on telegram/mailer/listener.py of __handle_mail(), in line
>>>> 77 it is already hold.
>>>
>>> Is this function only called from one place?
>>> If that's a guarantee, I'm fine with that. But why delete all files,
>>> it's the caller's problem that it doesn't call the function properly.
>>
>> It's also called from daemon/telegram/packages/client.py.
>>
>> That place doesn't hold the mutex anyway.
> 
> Correction, that place may hold or may not hold the mutex, depending
> where they are called. Another place that calls the remove_patch() is
> 
>     daemon/telegram/packages/plugins/commands/scrape.py
> 
> This one always doesn't hold the mutex.

I think deleting all files doesn't make so much sense to me.

Just delete the specific created file only. Why you can't do that?
Rizki, any comment?

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.33, Ammar Faizi wrote:
> On 10/29/22 2:22 AM, Muhammad Rizki wrote:
>> I create that statement because:
> [...]
>>          try:
>>              text, files, is_patch = utils.create_template(mail, 
>> Platform.TELEGRAM)
>>          except:
>>
>>              # ensure the created tmp dir is removed
>>              utils.remove_patch()
>>
>>              exc_str = utils.catch_err()
>>              self.client.logger.warning(exc_str)
>>              await self.client.send_log_file(url)
>>
>>              # return, to avoid local variable error
>>              return
> 
> This is ugly, will you do the:
> 
>      exc_str = utils.catch_err()
>      self.client.logger.warning(exc_str)
>      await self.client.send_log_file(url)
> 
> all over the place when error may happen?
> 

IDK, this is still debugging method. I don't have any idea when "all 
over the place when error may happen".

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.29, Alviro Iskandar Setiawan wrote:
> On Sat, Oct 29, 2022 at 2:15 AM Muhammad Rizki <kiizuha@gnuweeb.org> wrote:
>> Sorry, you mention remove_patch() which function from? Telegram bot
>> listener __send_mail() ? if yes, that function is already handled by
>> mutex, see it on telegram/mailer/listener.py of __handle_mail(), in line
>> 77 it is already hold.
> 
> Is this function only called from one place?

Which function? if __send_mail() yeah it only once called, the 
remove_patch() doesn't called once.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Sat, Oct 29, 2022 at 2:39 AM Muhammad Rizki wrote:
> Which function?

remove_patch().

> if __send_mail() yeah it only once called, the
> remove_patch() doesn't called once.

That's literally a hole. The problem is remove_patch() may be called
asynchronously together with a job that doesn't hold the mutex.

  - job_a is still using the file
  - job_b doesn't know anything about job_a, but it removes all files
blindly. job_b doesn't know anything about mutex in job_a.

-- Viro

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.38, Ammar Faizi wrote:
> 
> Just delete the specific created file only. Why you can't do that?
> Rizki, any comment?
> 

I would like to research it first later, I will return the generated 
temp dir and `return` it if after the gen_temp() is executed will throw 
an exception.

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.44, Alviro Iskandar Setiawan wrote:
> On Sat, Oct 29, 2022 at 2:39 AM Muhammad Rizki wrote:
>> Which function?
> 
> remove_patch().
> 
>> if __send_mail() yeah it only once called, the
>> remove_patch() doesn't called once.
> 
> That's literally a hole. The problem is remove_patch() may be called
> asynchronously together with a job that doesn't hold the mutex.
> 
>    - job_a is still using the file
>    - job_b doesn't know anything about job_a, but it removes all files
> blindly. job_b doesn't know anything about mutex in job_a.
> 
> -- Viro

So, you are talking about calling a remote_patch() all together if 
doesn't hold it with mutex?

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 1:15 AM, Muhammad Rizki wrote:
> Anyway, the rollback() is from the MySQL function?

Yes. Just a quick google. Please read:

https://pynative.com/python-mysql-transaction-management-using-commit-rollback/

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Ammar Faizi]

On 10/29/22 1:18 AM, Muhammad Rizki wrote:
> Anyway, I've ever wanted to refactor the DB using the SQLAlchemy and want to implement the clean-arch method too, but, IDK when to do that, maybe I could just re-create the daemon in my local and make the version 2 of it.

Not now, maybe next year.

For now, let's learn about ACID and how to handle error
properly first.

-- 
Ammar Faizi

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Alviro Iskandar Setiawan]

On Sat, Oct 29, 2022 at 2:46 AM Muhammad Rizki wrote:
> So, you are talking about calling a remote_patch() all together if
> doesn't hold it with mutex?

Yes, exactly. What else did you expect?

-- Viro

Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error

[Muhammad Rizki]

On 29/10/22 02.49, Ammar Faizi wrote:
> On 10/29/22 1:18 AM, Muhammad Rizki wrote:
>> Anyway, I've ever wanted to refactor the DB using the SQLAlchemy and 
>> want to implement the clean-arch method too, but, IDK when to do that, 
>> maybe I could just re-create the daemon in my local and make the 
>> version 2 of it.
> 
> Not now, maybe next year.
> 
> For now, let's learn about ACID and how to handle error
> properly first.
> 

Okay, just teach me how to do it, I really never do the ACID method, so 
yeah, please understand a bit.