Allow sanity check to pass without locking the cache

We check the sanity file once, without holding the lock, and if that
fails we acquire the lock and check again.

This means that if some other process is performing some long running
cache operation (e.g building libc) it won't block all emscripten
processes from running (e.g. compile processes won't be blocked by link
process).

Author: sbc100

emcc.py

@@ -3245,17 +3245,17 @@ def consume_arg_file():
     elif check_flag('--clear-cache'):
       logger.info('clearing cache as requested by --clear-cache: `%s`', shared.Cache.dirname)
       shared.Cache.erase()
-      shared.check_sanity(force=True) # this is a good time for a sanity check
+      shared.perform_sanity_checks() # this is a good time for a sanity check
       should_exit = True
     elif check_flag('--clear-ports'):
       logger.info('clearing ports and cache as requested by --clear-ports')
       ports.clear()
       shared.Cache.erase()
-      shared.check_sanity(force=True) # this is a good time for a sanity check
+      shared.perform_sanity_checks() # this is a good time for a sanity check
       should_exit = True
     elif check_flag('--check'):
       print(version_string(), file=sys.stderr)
-      shared.check_sanity(force=True)
+      shared.perform_sanity_checks()
       should_exit = True
     elif check_flag('--show-ports'):
       ports.show_ports()

tests/common.py

@@ -463,7 +463,7 @@ def set_temp_dir(self, temp_dir):
   def setUpClass(cls):
     super().setUpClass()
     print('(checking sanity from test runner)') # do this after we set env stuff
-    shared.check_sanity(force=True)
+    shared.perform_sanity_checks()
 
   def setUp(self):
     super().setUp()

tests/test_other.py

@@ -12290,3 +12290,11 @@ def test_bigint64array_polyfill(self):
 
     for m, [v1, v2] in output['assertEquals']:
       self.assertEqual(v1, v2, msg=m)
+
+  def test_compile_with_cache_lock(self):
+    # Verify that, after warming the cache, running emcc does not require the cache lock.
+    # Previously we would acquire the lock during sanity checking (even when the check
+    # passed) which meant the second process here would deadlock.
+    self.run_process([EMCC, '-c', test_file('hello_world.c')])
+    with shared.Cache.lock('testing'):
+      self.run_process([EMCC, '-c', test_file('hello_world.c')])

tools/cache.py

@@ -16,16 +16,6 @@
 
 # Permanent cache for system librarys and ports
 class Cache:
-  # If EM_EXCLUSIVE_CACHE_ACCESS is true, this process is allowed to have direct
-  # access to the Emscripten cache without having to obtain an interprocess lock
-  # for it. Generally this is false, and this is used in the case that
-  # Emscripten process recursively calls to itself when building the cache, in
-  # which case the parent Emscripten process has already locked the cache.
-  # Essentially the env. var EM_EXCLUSIVE_CACHE_ACCESS signals from parent to
-  # child process that the child can reuse the lock that the parent already has
-  # acquired.
-  EM_EXCLUSIVE_CACHE_ACCESS = int(os.environ.get('EM_EXCLUSIVE_CACHE_ACCESS', '0'))
-
   def __init__(self, dirname):
     # figure out the root directory for all caching
     self.dirname = Path(dirname).resolve()
@@ -37,42 +27,34 @@ def __init__(self, dirname):
     self.filelock_name = Path(dirname, 'cache.lock')
     self.filelock = filelock.FileLock(self.filelock_name)
 
-  def acquire_cache_lock(self):
+  def acquire_cache_lock(self, reason):
     if config.FROZEN_CACHE:
       # Raise an exception here rather than exit_with_error since in practice this
       # should never happen
       raise Exception('Attempt to lock the cache but FROZEN_CACHE is set')
 
-    if not self.EM_EXCLUSIVE_CACHE_ACCESS and self.acquired_count == 0:
+    if self.acquired_count == 0:
       logger.debug(f'PID {os.getpid()} acquiring multiprocess file lock to Emscripten cache at {self.dirname}')
       try:
         self.filelock.acquire(60)
       except filelock.Timeout:
-        # The multiprocess cache locking can be disabled altogether by setting EM_EXCLUSIVE_CACHE_ACCESS=1 environment
-        # variable before building. (in that case, use "embuilder.py build ALL" to prepopulate the cache)
-        logger.warning(f'Accessing the Emscripten cache at "{self.dirname}" is taking a long time, another process should be writing to it. If there are none and you suspect this process has deadlocked, try deleting the lock file "{self.filelock_name}" and try again. If this occurs deterministically, consider filing a bug.')
+        logger.warning(f'Accessing the Emscripten cache at "{self.dirname}" (for "{reason}") is taking a long time, another process should be writing to it. If there are none and you suspect this process has deadlocked, try deleting the lock file "{self.filelock_name}" and try again. If this occurs deterministically, consider filing a bug.')
         self.filelock.acquire()
 
-      self.prev_EM_EXCLUSIVE_CACHE_ACCESS = os.environ.get('EM_EXCLUSIVE_CACHE_ACCESS')
-      os.environ['EM_EXCLUSIVE_CACHE_ACCESS'] = '1'
       logger.debug('done')
     self.acquired_count += 1
 
   def release_cache_lock(self):
     self.acquired_count -= 1
     assert self.acquired_count >= 0, "Called release more times than acquire"
-    if not self.EM_EXCLUSIVE_CACHE_ACCESS and self.acquired_count == 0:
-      if self.prev_EM_EXCLUSIVE_CACHE_ACCESS:
-        os.environ['EM_EXCLUSIVE_CACHE_ACCESS'] = self.prev_EM_EXCLUSIVE_CACHE_ACCESS
-      else:
-        del os.environ['EM_EXCLUSIVE_CACHE_ACCESS']
+    if self.acquired_count == 0:
       self.filelock.release()
       logger.debug(f'PID {os.getpid()} released multiprocess file lock to Emscripten cache at {self.dirname}')
 
   @contextlib.contextmanager
-  def lock(self):
+  def lock(self, reason):
     """A context manager that performs actions in the given directory."""
-    self.acquire_cache_lock()
+    self.acquire_cache_lock(reason)
     try:
       yield
     finally:
@@ -82,7 +64,7 @@ def ensure(self):
     utils.safe_ensure_dirs(self.dirname)
 
   def erase(self):
-    with self.lock():
+    with self.lock('erase'):
       if self.dirname.exists():
         for f in os.listdir(self.dirname):
           tempfiles.try_delete(Path(self.dirname, f))
@@ -129,7 +111,7 @@ def erase_lib(self, name):
     self.erase_file(self.get_lib_name(name))
 
   def erase_file(self, shortname):
-    with self.lock():
+    with self.lock('erase: ' + shortname):
       name = Path(self.dirname, shortname)
       if name.exists():
         logger.info(f'deleting cached file: {name}')
@@ -153,7 +135,7 @@ def get(self, shortname, creator, what=None, force=False):
       # should never happen
       raise Exception(f'FROZEN_CACHE is set, but cache file is missing: "{shortname}" (in cache root path "{self.dirname}")')
 
-    with self.lock():
+    with self.lock(shortname):
       if cachename.exists() and not force:
         return str(cachename)
       if what is None:

tools/ports/__init__.py

@@ -175,7 +175,7 @@ def fetch_project(name, url, subdir, sha512hash=None):
     if local_ports:
       logger.warning('using local ports: %s' % local_ports)
       local_ports = [pair.split('=', 1) for pair in local_ports.split(',')]
-      with shared.Cache.lock():
+      with shared.Cache.lock('local ports'):
         for local in local_ports:
           if name == local[0]:
             path = local[1]
@@ -244,7 +244,7 @@ def up_to_date():
 
     # main logic. do this under a cache lock, since we don't want multiple jobs to
     # retrieve the same port at once
-    with shared.Cache.lock():
+    with shared.Cache.lock('unpack port'):
       if os.path.exists(fullpath):
         # Another early out in case another process build the library while we were
         # waiting for the lock

tools/shared.py

@@ -374,7 +374,7 @@ def perform_sanity_checks():
 
 
 @ToolchainProfiler.profile()
-def check_sanity(force=False):
+def check_sanity():
   """Check that basic stuff we need (a JS engine to compile, Node.js, and Clang
   and LLVM) exists.
 
@@ -383,19 +383,14 @@ def check_sanity(force=False):
   EM_CONFIG (so, we re-check sanity when the settings are changed).  We also
   re-check sanity and clear the cache when the version changes.
   """
-  if not force and os.environ.get('EMCC_SKIP_SANITY_CHECK') == '1':
+  if os.environ.get('EMCC_SKIP_SANITY_CHECK') == '1':
     return
 
   # We set EMCC_SKIP_SANITY_CHECK so that any subprocesses that we launch will
   # not re-run the tests.
   os.environ['EMCC_SKIP_SANITY_CHECK'] = '1'
 
-  if DEBUG:
-    force = True
-
   if config.FROZEN_CACHE:
-    if force:
-      perform_sanity_checks()
     return
 
   if os.environ.get('EM_IGNORE_SANITY'):
@@ -405,31 +400,37 @@ def check_sanity(force=False):
   expected = generate_sanity()
 
   sanity_file = Cache.get_path('sanity.txt')
-  with Cache.lock():
+
+  def sanity_is_correct():
     if os.path.exists(sanity_file):
       sanity_data = utils.read_file(sanity_file)
-      if sanity_data != expected:
-        logger.debug('old sanity: %s' % sanity_data)
-        logger.debug('new sanity: %s' % expected)
-        logger.info('(Emscripten: config changed, clearing cache)')
-        Cache.erase()
-        # the check actually failed, so definitely write out the sanity file, to
-        # avoid others later seeing failures too
-        force = False
-      else:
-        if force:
-          logger.debug(f'sanity file up-to-date but check forced: {sanity_file}')
-        else:
-          logger.debug(f'sanity file up-to-date: {sanity_file}')
-          return # all is well
+      if sanity_data == expected:
+        logger.debug(f'sanity file up-to-date: {sanity_file}')
+        return True # all is well
+    return False
+
+  if sanity_is_correct():
+    # Early return without taking the cache lock
+    return
+
+  with Cache.lock('sanity'):
+    # Check again once the cache lock as aquired
+    if sanity_is_correct():
+      return
+
+    if os.path.exists(sanity_file):
+      sanity_data = utils.read_file(sanity_file)
+      logger.debug('old sanity: %s' % sanity_data)
+      logger.debug('new sanity: %s' % expected)
+      logger.info('(Emscripten: config changed, clearing cache)')
+      Cache.erase()
     else:
       logger.debug(f'sanity file not found: {sanity_file}')
 
     perform_sanity_checks()
 
-    if not force:
-      # Only create/update this file if the sanity check succeeded, i.e., we got here
-      utils.write_file(sanity_file, expected)
+    # Only create/update this file if the sanity check succeeded, i.e., we got here
+    utils.write_file(sanity_file, expected)
 
 
 # Some distributions ship with multiple llvm versions so they add