search

catkin / catkin_tools

Command line tools for working with catkin
Apache License 2.0
163 stars 146 forks source link

Logger closing #700

Closed MatthijsBurgh closed 2 years ago

MatthijsBurgh commented 2 years ago

I am not experiencing any issues. But I was scrolling a bit through the code and found that in async_job the logger isn't closed, when there is any interruption, Exception, break, continue.

https://github.com/catkin/catkin_tools/blob/6d9f6fdc86c56850e521e6a665b89b033612a2f8/catkin_tools/execution/executor.py#L172-L179

I think this is incorrect. The logger should always be closed.

A finally will also be executed on a return, break, continue. See https://docs.python.org/3/reference/compound_stmts.html#finally

So by just moving the logger.close() to the finally should do the job.

MatthijsBurgh commented 2 years ago

@meyerj friendly ping ;)

MatthijsBurgh commented 2 years ago

@timonegk could you please take a quick look at this one. If I am wrong, we can just close this one.

timonegk commented 2 years ago

No, I'm pretty sure you're right. It would change the behavior a bit because the whole log file renaming and cleaning is also done in close(), but I think that would be for the best.

MatthijsBurgh commented 2 years ago

Each stage has its own logger. So in the old situation, the logger would be deleted after each stage iteration in case of not reaching the close call, which will then be after release the lock instead of before. So I think the behavioural change is very limited.

All tests in #719 succeed.

meyerj commented 2 years ago

@meyerj friendly ping ;)

Not sure why I have been pinged here? I am neither a maintainer nor contributor to that part of the code and not aware of how logging in catkin_tools works. However, I added a small suggestion to #719.

MatthijsBurgh commented 2 years ago

@meyerj I have really no idea why I have tagged you in this one 😕. Though thanks for your comment in #719.