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

[Ammar Faizi <ammarfaizi2@gnuweeb.org>]

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