sleep and retry on failure to delete temp file in tests

[amccarth]

Summary

This solves a flaky test on Windows.

We have similar logic elsewhere for log files. I considered trying to combine them, but since they are methods of different objects, there wasn't really a clean approach.

[zturner]

How about add a function to lldbsuite\support\fs.py?

def remove_file(file, num_retries, sleep_duration):

Then just call this everywhere you need to. Could even nuke this whole RemoveTempFile method.

[amccarth]

Consolidated the os.remove and os.unlink calls with a function to do the retries. Tested on Windows.

[zturner]

packages/Python/lldbsuite/test/lldbtest.py

1994 ↗

(On Diff #53100)

I think the default for num_retries should be 0. Usually you just want to try once without retry.

1995 ↗

(On Diff #53100)

You could do for i in range(num_retries+1): and get rid of a bunch of the counter updating and junk

[amccarth]

packages/Python/lldbsuite/test/lldbtest.py

1994 ↗

(On Diff #53100)

I disagree. Every call site was already doing exactly one retry. I don't see value in making each of them specify that explicitly.

1995 ↗

(On Diff #53100)

Fine, but that changes the behavior a bit. My approach avoided that extra sleep after the last retry.

[zturner]

Ahh, you're right, I didn't notice that.

lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py

def dumpSessionInfo(self):
             for src in log_files_for_this_test:
                 if os.path.isfile(src):
                     dst = src.replace(self.log_basename, dst_log_basename)
                     if os.name == "nt" and os.path.isfile(dst):
                         # On Windows, renaming a -> b will throw an exception if b exists.  On non-Windows platforms
                         # it silently replaces the destination.  Ultimately this means that atomic renames are not
                         # guaranteed to be possible on Windows, but we need this to work anyway, so just remove the
                         # destination first if it already exists.
-                        os.remove(dst)
+                        remove_file(dst)
 
                     os.rename(src, dst)
         else:
             # success!  (and we don't want log files) delete log files
             for log_file in log_files_for_this_test:
-                try:
-                    os.unlink(log_file)
-                except:
-                    # We've seen consistent unlink failures on Windows, perhaps because the
-                    # just-created log file is being scanned by anti-virus.  Empirically, this
-                    # sleep-and-retry approach allows tests to succeed much more reliably.
-                    # Attempts to figure out exactly what process was still holding a file handle
-                    # have failed because running instrumentation like Process Monitor seems to
-                    # slow things down enough that the problem becomes much less consistent.
-                    time.sleep(0.5)
-                    os.unlink(log_file)
+                remove_file(log_file)
 
     # ====================================================
     # Config. methods supported through a plugin interface
     # (enables reading of the current test configuration)
     # ====================================================
 
     def getArchitecture(self):
         """Returns the architecture in effect the test suite is running with."""
def DebugPExpect(self, child):
         if not traceAlways:
             return
 
         print(child)
 
     @classmethod
     def RemoveTempFile(cls, file):
         if os.path.exists(file):
+            remove_file(file)
+
+# On Windows, the first attempt to delete a recently-touched file can fail
+# because of a race with antimalware scanners.  This function will detect a
+# failure and retry.
+def remove_file(file, num_retries = 1, sleep_duration = 0.5):
+    for i in range(num_retries+1):
+        try:
             os.remove(file)
+            return True
+        except:
+            time.sleep(sleep_duration)
+            continue
+    return False