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

[Muhammad Rizki <kiizuha@gnuweeb.org>]

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.