Refactor the code of the background thread used in "fast delete" mode#286
Conversation
Complete the existing test, which had no `verify.groovy` file.
- Move the `FAST_MODE_*` string constants to an enumeration. - Move the code for background execution in a separated `BakgroundCleaner` subclass. - Use `java.util.concurrent.ExecutorService` instead of a thread with our own lifecycle management. - Add explanation about why a directory may be moved twice (same algorithm as before, just explained). - Reduce the number of directory levels by one (no need for a directory which will always contain exactly one directory). - Consolidation of exception handling, including the addition of error reporting at the end.
|
This pull request also fixes an issue: the |
|
Another minor issue fixed by this pull request is that the previous code attempted to delete the same directory twice, which caused a log on the second attempt saying the the plugin failed to delete a directory. |
| * @author Benjamin Bentmann | ||
| * @author Martin Desruisseaux | ||
| */ | ||
| final class BackgroundCleaner extends Cleaner implements Listener, Runnable { |
There was a problem hiding this comment.
Is final really needed here ?
There was a problem hiding this comment.
Not really needed. It is only because I find easier to analyse a code when there are less degrees of freedom (methods behaviour, field values, etc.). Since this class is not public, we do not need to worry about compatibility and can easily remove the final keyword when a subclass is needed.
| /** | ||
| * Errors that occurred during the deletion. | ||
| */ | ||
| private IOException errors; |
There was a problem hiding this comment.
error ? singular, since it contains only a single exception ?
There was a problem hiding this comment.
It can contain many exceptions as suppressed exceptions. I will clarify the documentation.
| if (errors == null) { | ||
| errors = e; | ||
| } else { | ||
| errors.addSuppressed(e); |
There was a problem hiding this comment.
I wonder if for ease of understanding, it would be better to create a container exception and add the previous one as a suppressed exception. That's how I've seen things so far.
There was a problem hiding this comment.
In the current proposal, the first exception is the container for all subsequent exceptions. This is the same behaviour as "try with resource" with two or more resources when exceptions occur in the calls to close(): resources are closed in reverse order, and the first exception is the container for other exceptions (this is true also when the first exception occurred inside the try block).
Admittedly, applying the same pattern to the Maven Clean Plugin is assuming that exceptions after the first one are likely to be indirect consequences of the first exceptions. It is hard to be sure that this assumption is true, but this uncertainty is also true for "try with resources".
I think that the intend is to have stack trace easier to analyse. Leaving apart the topic of checked exceptions, wrapping an exception in another exception is useful when we produce a more informative message. But often, we just put a "Failed to do XYZ" message, and users have to search deeper in the exception chain.
2 participants
Refactor the code of the background thread used in "fast delete" mode:
FAST_MODE_*string constants to an enumeration.BakgroundCleanersubclass.java.util.concurrent.ExecutorServiceinstead of a thread with our own lifecycle management.Purpose
Simplify the
Cleanerclass for the default case (when not using a background thread), have the background code in separated classes, try to make the code slightly safer (Enum,ExecutorService), prepare for the use of more than one thread in the future if we wish to do so (can be done by configuring theExecutorService).