bit-team / backintime

Back In Time - An easy-to-use backup tool for GNU/Linux using rsync in the back
https://backintime.readthedocs.io
GNU General Public License v2.0
2.09k stars 204 forks source link

MountLock context manager (was: Exception handling in mount/unmount; Enable lint check W0706) #1799

Open feman323 opened 4 months ago

feman323 commented 4 months ago

W0706 complains if an Exception is directly raised back without any handling. First I wanted to remove the try / catch blocks completely, but the I've seen that there are also "finally" statements. Therefore I just added some logging, which caused the lint check to run successful.

buhtz commented 4 months ago

Hi Manual, thank your for this contribution. I would set this as draft because it needs further investigation and resources. Your solution might silence the linter but does not improve the quality of code or BIT behavior. The log output is polluted without any reason.

There is more wrong then just a not handled exception. On a quick view I would assume that the goal of this code construct was that the code block in finally will be executed no matter if there is an exception or not. It might have been the only way in older Python versions but not in today Python.

Today you would solve something like this with a context manager.

But I am not sure at all and would need some time to dive into it.

Best, Christian

As an example you see here that the exception is re-raised but finally is also executed.

>>> try:
...   x = 7 / 0
... except Exception:
...    raise
... finally:
...   print('FF')
... 
FF
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
ZeroDivisionError: division by zero
>>> 
feman323 commented 4 months ago

Hi Christian,

Okay, I will try to embed this in a context manager.

Regards,

Manuel

buhtz commented 3 months ago

Please have a look if our (quit fresh) flock module and its base class _FlockContext might help you. I am assuming that base class need to be modified somehow to be more flexible.

buhtz commented 2 months ago

My advice would be to extract the five methods mountLock*() and mountProcess*() into something else. A context manager class for example.

feman323 commented 1 month ago

Hello Christian,

Sorry for the late response. At the moment, I don't have much time to work on this PR.

I have one question: Does it make sense to extract the mountLock methods into a context manager? The process lock prevents other processes from making changes at the same time and should always be removed after execution, so I understand why a context manager makes sense in this case.

However, the mountLock, on the other hand, should only be created / removed if the folder was successfully mounted / unmounted. So the methods calls for creating / removing the lock can be placed at the end of the mount / umount methods without the need for a context manager. Or am I missing something?

buhtz commented 1 month ago

Hello Manuel,

At the moment, I don't have much time to work on this PR.

That is OK. I just need to know.

I have one question: Does it make sense ...

However, the mountLock, on the other hand...

I don't have an answer or opinion about it. I am not deep enough into that topic. But me or someone else will consider your comment when picking up this PR again.

Best, Christian