Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/773#discussion_r129102424
  
    --- Diff: utils/common/src/main/java/org/apache/brooklyn/util/os/Os.java ---
    @@ -317,21 +318,28 @@ public void run() throws IOException {
         private static final Map<String,FileDeletionHook> deletions = new 
LinkedHashMap<String, Os.FileDeletionHook>();
         
         private static void addShutdownFileDeletionHook(String path, 
FileDeletionHook hook) {
    +        // ensure Ordering class is loaded (else shutdown hook will fail 
as it can't load that class when shutting down)
    +        Ordering.<Integer>natural();
             synchronized (deletions) {
                 if (deletions.isEmpty()) {
                     Thread shutdownHook = new Thread() {
                         @Override
                         public void run() {
    -                        synchronized (deletions) {
    -                            List<String> pathsToDelete = new 
ArrayList<String>(deletions.keySet());
    -                            Collections.sort(pathsToDelete, 
Strings.lengthComparator().reverse());
    -                            for (String path: pathsToDelete) {
    -                                try {
    -                                    deletions.remove(path).run();
    -                                } catch (Exception e) {
    -                                    log.warn("Unable to delete '"+path+"' 
on shutdown: "+e);
    +                        log.debug("Shutting down, deleting: "+deletions);
    +                        try {
    +                            synchronized (deletions) {
    +                                List<String> pathsToDelete = new 
ArrayList<String>(deletions.keySet());
    +                                Collections.sort(pathsToDelete, 
Strings.lengthComparator().reverse());
    +                                for (String path: pathsToDelete) {
    +                                    try {
    +                                        deletions.remove(path).run();
    +                                    } catch (Exception e) {
    +                                        log.warn("Unable to delete 
'"+path+"' on shutdown: "+e);
    +                                    }
                                     }
                                 }
    +                        } catch (Exception e) {
    --- End diff --
    
    I'd prefer to include `, e`, rather than just its toString. For example, if 
there were an NPE somewhere in the method then we'd only be told that there was 
an NPE rather than being told its line number etc.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---

Reply via email to