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

[feman323]

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]

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]

Hi Christian,

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

Regards,

Manuel

[buhtz]

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]

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

[feman323]

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]

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

[buhtz]

Hello Manuel, I got deeper in the related code around mount locking to better understand how it works and what it does. In PR #1942 I added some unit tests and refactored code.

The mounting is a very sensible and important part of the BIT codebase. There is too much going on under the hood and I am still not able to estimate the implications for our users. Taking risk-benefit calculation of your PR into account I can sleep better if we don't merge.

My apologize for this.

Keep in mind that your work is not waste. It triggered some other actions (e.g. PR #1942 ) improving BIT. So thank you for this.

Regards, Christian

[buhtz]

Closing this PR based on the comment above. Feel free to reopen. Thank you for your efforts. If you have any further questions, ideas or encounter any other issues, please don't hesitate to let us know.

Best regards,