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

.circleci/config.yml

@@ -538,13 +538,7 @@ jobs:
           # skip other.test_stack_switching_size as we do not currently install
           # d8 on mac
           test_targets: "
-            other
-            sockets.test_nodejs_sockets_echo*
-            core0.test_lua
-            core0.test_longjmp_standalone
-            core2.test_sse1
-            skip:other.test_native_link_error_message
-            skip:other.test_stack_switching_size"
+            other.test_recursive_cache_lock"
 
 workflows:
   build-test:

emcc.py

@@ -3274,16 +3274,19 @@ def consume_arg_file():
     elif check_arg('--cache'):
       config.CACHE = os.path.normpath(consume_arg())
       shared.reconfigure_cache()
+      # Ensure child processes share the same cache (e.g. when using emcc to compiler system
+      # libraries)
+      os.environ['EM_CACHE'] = config.CACHE
     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)

test/test_other.py

@@ -12406,3 +12406,20 @@ def test_warn_once(self):
       }
     ''')
     self.do_runf('main.c', 'warning: foo\ndone\n')
+
+  @with_env_modify({'EM_FROZEN_CACHE': ''})
+  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')])
+    config.FROZEN_CACHE = False
+    with shared.Cache.lock('testing'):
+      self.run_process([EMCC, '-c', test_file('hello_world.c')])
+
+  @with_env_modify({'EM_FROZEN_CACHE': ''})
+  def test_recursive_cache_lock(self):
+    config.FROZEN_CACHE = False
+    with shared.Cache.lock('testing'):
+      err = self.run_process([EMBUILDER, 'build', 'libc', '--force'], stderr=PIPE, check=False).stderr
+    self.assertContained('AssertionError: attempt to lock the cache while a parent process is holding the lock', err)

tools/cache.py

@@ -16,15 +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
@@ -37,42 +28,38 @@ 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}')
+      assert 'EM_CACHE_IS_LOCKED' not in os.environ, f'attempt to lock the cache while a parent process is holding the lock ({reason})'
       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'
+      os.environ['EM_CACHE_IS_LOCKED'] = '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:
+      assert os.environ['EM_CACHE_IS_LOCKED'] == '1'
+      del os.environ['EM_CACHE_IS_LOCKED']
       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 +69,7 @@ def ensure(self):
     utils.safe_ensure_dirs(self.dirname)
 
   def erase(self):
-    with self.lock():
+    with self.lock('erase'):
       # Delete everything except the lockfile itself
       utils.delete_contents(self.dirname, exclude=[os.path.basename(self.filelock_name)])
 
@@ -128,7 +115,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}')
@@ -152,7 +139,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

@@ -174,7 +174,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]
@@ -243,7 +243,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

@@ -389,6 +389,8 @@ def check_sanity(force=False):
   # not re-run the tests.
   os.environ['EMCC_SKIP_SANITY_CHECK'] = '1'
 
+  # In DEBUG mode we perform the sanity checks even when
+  # early return due to the file being up-to-date.
   if DEBUG:
     force = True
 
@@ -404,31 +406,39 @@ 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 sanity_data == expected:
+        logger.debug(f'sanity file up-to-date: {sanity_file}')
         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
+          perform_sanity_checks()
+        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