Switch timeout mechanism to subprocess.run (#659)

* Switch to subprocess.run for timeouts

* [experiment] Set CI TIMEOUT_SCALE_FACTOR to 1

* supress parameterized.expand error in event of no compilers, change TIMEOUT_SCALE_FACTOR in CI to 2

* Exempt dummy compilers from nsjail bind mounts, disable nsjail time limits

* Don't run timeout test on windows

* Decouple test_compiler_timeout's timeout from global timeout settings, reset TIMEOUT_SCALE_FACTOR to 10

* Have DummyLongRunningCompiler subclass DummyCompiler

* Have a timeout of zero disable timeouts entirely

* Skip test_zero_timeout on windows

* black

Co-authored-by: ConorBobbleHat <c.github@firstpartners.net>

backend/coreapp/compiler_wrapper.py

@@ -15,7 +15,6 @@
 
 from coreapp.platforms import Platform
 import coreapp.util as util
-from coreapp.util import exception_on_timeout
 
 from .error import AssemblyError, CompilationError
 from .models.scratch import Asm, Assembly
@@ -164,11 +163,11 @@ def compile_code(
             # Run compiler
             try:
                 st = round(time.time() * 1000)
-                compile_proc = exception_on_timeout(
-                    settings.COMPILATION_TIMEOUT_SECONDS
-                )(sandbox.run_subprocess)(
+                compile_proc = sandbox.run_subprocess(
                     cc_cmd,
-                    mounts=[compiler.path],
+                    mounts=(
+                        [compiler.path] if compiler.platform != platforms.DUMMY else []
+                    ),
                     shell=True,
                     env={
                         "PATH": PATH,
@@ -182,6 +181,7 @@ def compile_code(
                         "MWCIncludes": "/tmp",
                         "TMPDIR": "/tmp",
                     },
+                    timeout=settings.COMPILATION_TIMEOUT_SECONDS,
                 )
                 et = round(time.time() * 1000)
                 logging.debug(f"Compilation finished in: {et - st} ms")
@@ -195,8 +195,8 @@ def compile_code(
                 # Shlex issue?
                 logging.debug("Compilation failed: %s", e)
                 raise CompilationError(str(e))
-            except TimeoutError as e:
-                raise CompilationError(str(e))
+            except subprocess.TimeoutExpired as e:
+                raise CompilationError("Compilation failed: timeout expired")
 
             if not object_path.exists():
                 error_msg = (
@@ -245,9 +245,7 @@ def assemble_asm(platform: Platform, asm: Asm) -> Assembly:
 
             # Run assembler
             try:
-                assemble_proc = exception_on_timeout(settings.ASSEMBLY_TIMEOUT_SECONDS)(
-                    sandbox.run_subprocess
-                )(
+                assemble_proc = sandbox.run_subprocess(
                     platform.assemble_cmd,
                     mounts=[],
                     shell=True,
@@ -256,11 +254,12 @@ def assemble_asm(platform: Platform, asm: Asm) -> Assembly:
                         "INPUT": sandbox.rewrite_path(asm_path),
                         "OUTPUT": sandbox.rewrite_path(object_path),
                     },
+                    timeout=settings.ASSEMBLY_TIMEOUT_SECONDS,
                 )
             except subprocess.CalledProcessError as e:
                 raise AssemblyError.from_process_error(e)
-            except TimeoutError as e:
-                raise AssemblyError(str(e))
+            except subprocess.TimeoutExpired as e:
+                raise AssemblyError("Timeout expired")
 
             # Assembly failed
             if assemble_proc.returncode != 0:

backend/coreapp/compilers.py

@@ -33,6 +33,8 @@
     SWITCH,
 )
 
+import platform as platform_stdlib
+
 logger = logging.getLogger(__name__)
 
 CONFIG_PY = "config.py"
@@ -98,6 +100,12 @@ def available(self) -> bool:
         return settings.DUMMY_COMPILER
 
 
+@dataclass(frozen=True)
+class DummyLongRunningCompiler(DummyCompiler):
+    def available(self) -> bool:
+        return settings.DUMMY_COMPILER and platform_stdlib.system() != "Windows"
+
+
 @dataclass(frozen=True)
 class ClangCompiler(Compiler):
     flags: ClassVar[Flags] = COMMON_CLANG_FLAGS
@@ -167,6 +175,10 @@ def preset_from_name(name: str) -> Optional[Preset]:
 
 DUMMY = DummyCompiler(id="dummy", platform=platforms.DUMMY, cc="")
 
+DUMMY_LONGRUNNING = DummyLongRunningCompiler(
+    id="dummy_longrunning", platform=platforms.DUMMY, cc="sleep 3600"
+)
+
 # GBA
 AGBCC = GCCCompiler(
     id="agbcc",
@@ -699,6 +711,7 @@ def preset_from_name(name: str) -> Optional[Preset]:
 
 _all_compilers: List[Compiler] = [
     DUMMY,
+    DUMMY_LONGRUNNING,
     # GBA
     AGBCC,
     OLD_AGBCC,

backend/coreapp/decompiler_wrapper.py

@@ -6,7 +6,6 @@
 
 from coreapp.m2c_wrapper import M2CError, M2CWrapper
 from coreapp.platforms import Platform
-from coreapp.util import exception_on_timeout
 from django.conf import settings
 
 logger = logging.getLogger(__name__)
@@ -31,11 +30,7 @@ def decompile(
             if len(asm.splitlines()) > MAX_M2C_ASM_LINES:
                 return "/* Too many lines to decompile; please run m2c manually */"
             try:
-                ret = exception_on_timeout(settings.DECOMPILATION_TIMEOUT_SECONDS)(
-                    M2CWrapper.decompile
-                )(asm, context, compiler, platform.arch)
-            except TimeoutError as e:
-                ret = f"/* Timeout error while running m2c */\n{default_source_code}"
+                ret = M2CWrapper.decompile(asm, context, compiler, platform.arch)
             except M2CError as e:
                 ret = f"{e}\n{default_source_code}"
             except Exception:

backend/coreapp/diff_wrapper.py

@@ -8,7 +8,6 @@
 
 from coreapp.platforms import DUMMY, Platform
 from coreapp.flags import ASMDIFF_FLAG_PREFIX
-from coreapp.util import exception_on_timeout
 from django.conf import settings
 
 from .compiler_wrapper import DiffResult, PATH
@@ -100,17 +99,16 @@ def get_objdump_target_function_flags(
             raise NmError(f"No nm command for {platform.id}")
 
         try:
-            nm_proc = exception_on_timeout(settings.OBJDUMP_TIMEOUT_SECONDS)(
-                sandbox.run_subprocess
-            )(
+            nm_proc = sandbox.run_subprocess(
                 [platform.nm_cmd] + [sandbox.rewrite_path(target_path)],
                 shell=True,
                 env={
                     "PATH": PATH,
                 },
+                timeout=settings.OBJDUMP_TIMEOUT_SECONDS,
             )
-        except TimeoutError as e:
-            raise NmError(str(e))
+        except subprocess.TimeoutExpired as e:
+            raise NmError("Timeout expired")
         except subprocess.CalledProcessError as e:
             raise NmError.from_process_error(e)
 
@@ -165,19 +163,18 @@ def run_objdump(
 
             if platform.objdump_cmd:
                 try:
-                    objdump_proc = exception_on_timeout(
-                        settings.OBJDUMP_TIMEOUT_SECONDS
-                    )(sandbox.run_subprocess)(
+                    objdump_proc = sandbox.run_subprocess(
                         platform.objdump_cmd.split()
                         + flags
                         + [sandbox.rewrite_path(target_path)],
                         shell=True,
                         env={
                             "PATH": PATH,
                         },
+                        timeout=settings.OBJDUMP_TIMEOUT_SECONDS,
                     )
-                except TimeoutError as e:
-                    raise ObjdumpError(str(e))
+                except subprocess.TimeoutExpired as e:
+                    raise ObjdumpError("Timeout expired")
                 except subprocess.CalledProcessError as e:
                     raise ObjdumpError.from_process_error(e)
             else:

backend/coreapp/sandbox.py

@@ -69,9 +69,7 @@ def sandbox_command(self, mounts: List[Path], env: Dict[str, str]) -> List[str]:
             "--env", "PATH=/usr/bin:/bin",
             "--cwd", "/tmp",
             "--rlimit_fsize", "soft",
-             "--rlimit_nofile", "soft",
-            "--rlimit_cpu", "30",  # seconds
-            "--time_limit", "30",  # seconds
+            "--rlimit_nofile", "soft",
             # the following are settings that can be removed once we are done with wine
             "--bindmount_ro", f"{settings.WINEPREFIX}:/wine",
             "--env", "WINEDEBUG=-all",
@@ -98,9 +96,11 @@ def run_subprocess(
         mounts: Optional[List[Path]] = None,
         env: Optional[Dict[str, str]] = None,
         shell: bool = False,
+        timeout: Optional[float] = None,
     ) -> subprocess.CompletedProcess[str]:
         mounts = mounts if mounts is not None else []
         env = env if env is not None else {}
+        timeout = None if timeout == 0 else timeout
 
         try:
             wrapper = self.sandbox_command(mounts, env)
@@ -129,4 +129,5 @@ def run_subprocess(
             shell=False,
             stdout=subprocess.PIPE,
             stderr=subprocess.STDOUT,
+            timeout=timeout,
         )

backend/coreapp/tests.py

@@ -12,7 +12,8 @@
 from django.urls import reverse
 from rest_framework import status
 from rest_framework.test import APITestCase
-from coreapp.compilers import Language
+from coreapp.compilers import DummyCompiler, Language
+from coreapp.sandbox import Sandbox
 from coreapp import compilers, platforms
 
 from coreapp.compiler_wrapper import CompilerWrapper
@@ -523,7 +524,7 @@ def test_dummy_compiler(self) -> None:
             len(result.elf_object), 0, "The compilation result should be non-null"
         )
 
-    @parameterized.expand(input=[(c,) for c in compilers.available_compilers()])  # type: ignore
+    @parameterized.expand(input=[(c,) for c in compilers.available_compilers() if not isinstance(c, DummyCompiler)], skip_on_empty=True)  # type: ignore
     def test_all_compilers(self, compiler: Compiler) -> None:
         """
         Ensure that we can run a simple compilation/diff for all available compilers
@@ -556,6 +557,50 @@ def test_all_compilers(self, compiler: Compiler) -> None:
         self.assertGreater(len(diff["rows"]), 0)
 
 
+class TimeoutTests(BaseTestCase):
+    @requiresCompiler(compilers.DUMMY_LONGRUNNING)
+    def test_compiler_timeout(self) -> None:
+        # Test that a hanging compilation will fail with a timeout error
+        with self.settings(COMPILATION_TIMEOUT_SECONDS=3):
+            scratch_dict = {
+                "compiler": compilers.DUMMY_LONGRUNNING.id,
+                "platform": platforms.DUMMY.id,
+                "context": "",
+                "target_asm": "asm(AAAAAAAA)",
+            }
+
+            scratch = self.create_scratch(scratch_dict)
+
+            compile_dict = {
+                "slug": scratch.slug,
+                "compiler": compilers.DUMMY_LONGRUNNING.id,
+                "compiler_flags": "",
+                "source_code": "source(AAAAAAAA)",
+            }
+
+            response = self.client.post(
+                reverse("scratch-compile", kwargs={"pk": scratch.slug}), compile_dict
+            )
+
+            self.assertFalse(response.json()["success"])
+            self.assertIn("timeout expired", response.json()["compiler_output"].lower())
+
+    # if we don't have DUMMY_LONGRUNNING, it means we'll be unable to use sandbox.run_subprocess
+    @requiresCompiler(compilers.DUMMY_LONGRUNNING)
+    def test_zero_timeout(self) -> None:
+        # Tests that passing a timeout of zero to sandbox.run_subprocess will equate
+        # to disabling the timeout entirely
+        expected_output = "AAAAAAAA"
+
+        with Sandbox() as sandbox:
+            sandboxed_proc = sandbox.run_subprocess(
+                f"sleep 3 && echo {expected_output}", timeout=0, shell=True
+            )
+
+            self.assertEqual(sandboxed_proc.returncode, 0)
+            self.assertIn(expected_output, sandboxed_proc.stdout)
+
+
 class DecompilationTests(BaseTestCase):
     @requiresCompiler(GCC281)
     def test_default_decompilation(self) -> None:

backend/coreapp/util.py

@@ -1,20 +1,8 @@
 import hashlib
 import logging
 import time
-import dill
 
-import django
-
-import multiprocessing
-import functools
-import platform
-
-# For reasons of thread safety, guincorn refuses to let us join processes forked from a worker thread
-# To get around this, we opt to spawn a fresh process instead.
-mp = multiprocessing.get_context("spawn")
-
-from typing import Tuple, TypeVar, Callable, Any, cast
-from queue import Queue
+from typing import Tuple
 
 logger = logging.getLogger(__name__)
 
@@ -24,54 +12,3 @@
 
 def gen_hash(key: Tuple[str, ...]) -> str:
     return hashlib.sha256(str(key + (_startup_time,)).encode("utf-8")).hexdigest()
-
-
-F = TypeVar("F", bound=Callable[..., Any])
-
-# Python 3.10+ should allow this to be typed more concretely
-# (see https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators)
-
-# Windows requires multiprocessing processes to be in top-level scope
-def worker(queue: Queue[Any], func: bytes, args: Any, kwargs: Any) -> Any:
-    try:
-        # As we're in a new, spawn'ed environment, we have to do the bare minimum initalization ourselved
-        # (i.e. the django app registry)
-        django.setup()
-
-        ret = dill.loads(func)(*args, **kwargs)
-        queue.put(ret)
-    except Exception as e:
-        queue.put(e)
-
-
-def exception_on_timeout(timeout_seconds: float) -> Callable[[F], F]:
-    def timeout_inner(func: F) -> F:
-        @functools.wraps(func)
-        def wrapper(*args: Any, **kwargs: Any) -> Any:
-            # If the timeout is 0 or less, call the function directly without a timeout
-            if timeout_seconds <= 0:
-                return func(*args, **kwargs)
-
-            queue: Queue[Any] = mp.Queue()
-
-            # On Windows, multiprocessing uses pickle under the hood to serialize arguments
-            # It doesn't play nicely with arbitary functions, so we explicitly use its
-            # more versatile cousin (dill) to handle the serialization ourselves
-            p = mp.Process(target=worker, args=(queue, dill.dumps(func), args, kwargs))
-            p.start()
-            p.join(timeout_seconds)
-
-            if p.is_alive():
-                # The process has hanged - terminate, and throw an error
-                p.terminate()
-                p.join()
-                raise TimeoutError("Process timed out")
-            else:
-                ret = queue.get()
-                if isinstance(ret, Exception):
-                    raise ret
-                return ret
-
-        return cast(F, wrapper)
-
-    return timeout_inner

backend/decompme/settings.py

@@ -33,7 +33,6 @@
     COMPILATION_CACHE_SIZE=(int, 100),
     WINEPREFIX=(str, "/tmp/wine"),
     COMPILATION_TIMEOUT_SECONDS=(int, 10),
-    DECOMPILATION_TIMEOUT_SECONDS=(int, 5),
     ASSEMBLY_TIMEOUT_SECONDS=(int, 3),
     OBJDUMP_TIMEOUT_SECONDS=(int, 3),
     TIMEOUT_SCALE_FACTOR=(int, 1),
@@ -211,8 +210,5 @@
 COMPILATION_TIMEOUT_SECONDS = (
     env("COMPILATION_TIMEOUT_SECONDS", int) * TIMEOUT_SCALE_FACTOR
 )
-DECOMPILATION_TIMEOUT_SECONDS = (
-    env("DECOMPILATION_TIMEOUT_SECONDS", int) * TIMEOUT_SCALE_FACTOR
-)
 ASSEMBLY_TIMEOUT_SECONDS = env("ASSEMBLY_TIMEOUT_SECONDS", int) * TIMEOUT_SCALE_FACTOR
 OBJDUMP_TIMEOUT_SECONDS = env("OBJDUMP_TIMEOUT_SECONDS", int) * TIMEOUT_SCALE_FACTOR