Project

General

Profile

Bug #5744

failure processing shutdown hook to delete a temp directory

Added by Greg Shah over 2 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Vladimir Tsichevski
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version:

History

#1 Updated by Greg Shah over 2 years ago

  • Assignee set to Vladimir Tsichevski

Vladimir found issues with the shudown hook processing in #5034-1312.

Please do propose a fix for the shutdown hook. It definitely needs to be better.

I don't understand why there are 8 registered shutdown hooks. Is each one deleting a different directory?

#2 Updated by Constantin Asofiei over 2 years ago

I've fixed the abend in 3821c/13086, I forgot to mention on #5034.

The problem was that I was using a static field instead of a context-local field in p2j.util.Utils. So all clients were using the same temporary folder name.

#3 Updated by Vladimir Tsichevski over 2 years ago

Greg Shah wrote:

Vladimir found issues with the shudown hook processing in #5034-1312.

Please do propose a fix for the shutdown hook. It definitely needs to be better.

I don't understand why there are 8 registered shutdown hooks. Is each one deleting a different directory?

No all 8 are deleting the same directory, and this causes problems. I do not know why is this.

#4 Updated by Constantin Asofiei over 2 years ago

Vladimir Tsichevski wrote:

No all 8 are deleting the same directory, and this causes problems. I do not know why is this.

Actually now I've remembered that the code is really on the FWD client, so why you are getting the same random suffix?

And my 13086 change is not needed after all.

#5 Updated by Vladimir Tsichevski over 2 years ago

  • Status changed from New to WIP

The problem root is in this code fragment:

      if (!tmpDirObj.exists())
      {
         tmpDirObj.mkdir();
      }
      // adding file separator to the end
      tmpDir = tmpDir.concat(File.separator);

      // add a JVM hook to delete this folder (and all its content).
      Runtime.getRuntime().addShutdownHook(new Thread(() ->
      {
         int res = FileSystemDaemon.delete(tmpDirObj, true);

         if (res != FileSystem.ERR_NO_ERROR)
         {
            String path = tmpDirObj.getAbsolutePath();
            LOG.log(Level.SEVERE, "Error " + res + " while removing the FWD temporary folder: " + path);
         }
      }));

The directory is created only if it does not exist, but the cleanup hook is added as many times as the getOrCreateTemporaryDirectory() method is executed.

#6 Updated by Vladimir Tsichevski over 2 years ago

  • Status changed from WIP to Review
  • % Done changed from 0 to 100

Fixed in 3821c rev. 13089.

#7 Updated by Greg Shah over 2 years ago

Code Review Task Branch 3821c Revision 13089

The core change is fine.

The only issue I have is that you've revmoved some else branches that were there intentionally. Although your change doesn't have a functional problem, it leaves the code in a less readable form. With the else branches, you do not need to look inside the logic of the if to see that these two blocks are mutually exclusive. With the else removed, the reader must look at the if branch logic to determine that these if branches always exit. In addition, if someone changes the if branch to not exit, they must not forget to re-insert the else branch. I think that makes the code more error prone during future maintenance. Please put those else branches back in.

#8 Updated by Vladimir Tsichevski over 2 years ago

Greg Shah wrote:

Code Review Task Branch 3821c Revision 13089

The core change is fine.

The only issue I have is that you've revmoved some else branches that were there intentionally. Although your change doesn't have a functional problem, it leaves the code in a less readable form. With the else branches, you do not need to look inside the logic of the if to see that these two blocks are mutually exclusive. With the else removed, the reader must look at the if branch logic to determine that these if branches always exit. In addition, if someone changes the if branch to not exit, they must not forget to re-insert the else branch. I think that makes the code more error prone during future maintenance. Please put those else branches back in.

Restored in rev. 13090.

PS. I myself always prefer early exits (and Eclipse agrees with me :-), it regards unnecessary "else" a problem ).

#9 Updated by Greg Shah over 2 years ago

  • Status changed from Review to Closed

Also available in: Atom PDF