From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on gnuweeb.org X-Spam-Level: X-Spam-Status: No, score=-1.8 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,NO_DNS_FOR_FROM, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from [10.7.7.2] (unknown [182.253.183.90]) by gnuweeb.org (Postfix) with ESMTPSA id A957D804FD; Fri, 28 Oct 2022 16:29:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gnuweeb.org; s=default; t=1666974554; bh=G0IeZ9+PVfhAKAdGGR6WvRoL6hkgMOdqxq57E2hUNYg=; h=Date:From:To:Cc:References:Subject:In-Reply-To:From; b=tBqJ0l1RlG+x1tiKSUsG1DxOsykY3hTa8o4POHs1qHFJwGs43sIsEYJGcv7FtBXsj 9pKQ7I7N87c+oGbym3IpLySfthHAfo2kgVVfp0AC9O0Qpj6U91+RGpd0MRmyNpRbTG 3l3Chn4AiOzfPFuhWTGach/jnOG34z0wa9CgZzzQza13vdKDYqnULEItv9rBARChcl SL8mFzcgL7jMxA1lA/AsggkZ7mmE3iO8AzsgLN1ok2IVavxF02buvNszdqrlw8Upsg kSSG/7ODIM32xbSi+Vi2m/GF0Z+f5TKg/VnUZDfR+RjVTgJgDW3Nt0IRdv4JzSrj/x ij7CVyofJLC2Q== Message-ID: <043f55c3-67d8-9130-aca4-73c59926d2af@gnuweeb.org> Date: Fri, 28 Oct 2022 23:29:10 +0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Content-Language: en-US From: Ammar Faizi To: Alviro Iskandar Setiawan Cc: GNU/Weeb Mailing List , Muhammad Rizki References: <20221027150823.601914-1-ammarfaizi2@gnuweeb.org> <20221027150823.601914-3-ammarfaizi2@gnuweeb.org> <1d500d37-b11b-75fd-38e5-d7f8e0a9b1d4@gnuweeb.org> <3a79a587-ddee-9e25-2ac5-b573938b44a9@gnuweeb.org> Subject: Re: [PATCH v1 2/2] daemon: telegram: Handle MySQL error In-Reply-To: <3a79a587-ddee-9e25-2ac5-b573938b44a9@gnuweeb.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: 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