[Feature] Clean up temporary files generated by gcov tool when exception occurs. #1106
Conversation
|
Thanks for working on this but I think there is an easier way to I need to check it after my vacation on my computer. |
src/gcovr/formats/gcov/read.py
Outdated
| if filepath in temporary_files: | ||
| temporary_files.remove(filepath) |
There was a problem hiding this comment.
This for loop should be split. In line 794 after calling select_gcov_files_from_stdout we should delete the files list(all_gcov_files - active_gcov_files) and at the end in a finally block we should delete all files in active_gcov_files unless options.gcov_keep is set or we are not done.
In that case we do not need the temporary_files. Correct me if I'm wrong here.
There was a problem hiding this comment.
In this way, I think we are going to delete active_gcov_files in each thread when exception occurs.
(The following part is partially wrong, please refer to the latest comment)
However, according to my understanding, an exception will only be captured once in a multi-thread process, that is to say, only one thread that capture the exception will go to the finally block and delete those active_gcov_files generated by the thread and keep those active_gcov_files generated by other threads remain.
Therefore, in my implementation, I don't handle the exception in each worker thread, and only capture it in the main thread in line 95. In this way, we can ensure only the main thread will capture the exception and delete those active_gcov_files through temporary_files
There was a problem hiding this comment.
Hi, here's some update about the exception in a python multi-threading process after my survey and manual test with some python script
A signal ( KeyboardInterrupt is SIGINT, one of the signal to process ) will only be sent to the main thread in a python multi-threading process. See python official document on signal vs thread for more information.
Therefore, if we handle these exceptions in worker threads, those finally block won't be triggered if an keyboardInterrupt occurs.
There was a problem hiding this comment.
On keyboard interrupt it's fine for me to left the file because the user wants to abort (same as sending SIG_TERM or SIG_KILL).
We should delete the files as soon and as local as possible. Also if someone wants to keep the files why shall they be deleted if an interrupt occurs?
There was a problem hiding this comment.
I see. But is there a possible situation that someone wants to keep the files when they interrupt on their own? Those files are automatically removed by worker threads in just few seconds even if user not interrupt the files.
There was a problem hiding this comment.
Also if someone wants to keep the files why shall they be deleted if an interrupt occurs
I want to know more about this consideration. If all files are removed at the end of the process, users are not easily to know what files will be kept when an interrupt occurs, then in what situation, users may want to keep the files?
There was a problem hiding this comment.
If the user sets options.gcov_keep he wants to keep the files. If we remove all not used files direct after creation the user can be sure that the present files are used.
There was a problem hiding this comment.
I see. Then if the use doesn't set options.gcov_keep and the user abort gcovr, should we remove active_gcov_file ?
Take myself, as a user of gcovr, for example, I sometimes aborted gcovr and before my next run, I need to remove temporary files manually. So I recommend to remove these active_gcov_file if user doestn't set options.gcov_keep
There was a problem hiding this comment.
The active files are removed after the processing of the current gcda file. Why do you need to remove the files before the next run?
There was a problem hiding this comment.
I pushed on your branch. Now on keyboard interrupt the pool is drained and all the files are removed depending on the option --gcov-keep. Please can you check this on your site?
|
@kenchung285 Can you reply to the comments? |
|
Hi @Spacetown, I have replied to those comments and ping you for review. |
|
I've resubmit those comments and they seems great now. PTAL, thanks! |
You pressed the "Start review" button and in that case the comments are pending until you finish the review. |
Forcefully stop on keyboard interrupts.
|
Ple 8000 ase add yourself to the list of contributors and update the CHANGELOG. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Signed-off-by: Cheng-Yeh Chung <kenchung285@gmail.com>
Sorry for late reply. I've update the CHANGELOG and I've been added to the list of authors in #1075 |
|
Does it work for you? |
LGTM. But I would like to have some manual tests this weekend, is that okay with you? |
@Spacetown I've test this PR with my small testing project. It looks good! |
gcovr#1106) - Forcefully stop on keyboard interrupts. Signed-off-by: Cheng-Yeh Chung <kenchung285@gmail.com> Co-authored-by: Spacetown <40258682+spacetown@users.noreply.github.com>
Related Issue
Please see #1096 for more information
Tests
Due to #1105 errors in the latest main branch, this commits are manually tested on gcovr
8.3. The manual test includes multi-thread situations.Close Issue
Closes #1096