Merge pull request #249 from cs50/develop

v3.2.1

check50/runner.py

@@ -6,8 +6,10 @@
 import inspect
 import importlib
 import gettext
+import multiprocessing
 import os
 from pathlib import Path
+import pickle
 import shutil
 import signal
 import sys
@@ -17,7 +19,7 @@
 import attr
 import lib50
 
-from . import internal, __version__
+from . import internal, _exceptions, __version__
 from ._api import log, Failure, _copy, _log, _data
 
 _check_names = []
@@ -238,7 +240,7 @@ def dependencies_of(self, targets):
         deps = set()
         for target in targets:
             if target not in inverse_graph:
-                raise internal.Error(_("Unknown check: {}").format(e.args[0]))
+                raise _exceptions.Error(_("Unknown check: {}").format(e.args[0]))
             curr_check = target
             while curr_check is not None and curr_check not in deps:
                 deps.add(curr_check)
@@ -338,7 +340,43 @@ def __init__(self, check_name, spec, state=None):
         self.check_name = check_name
         self.spec = spec
         self.state = state
-        self.attribute_values = tuple(eval(name) for name in self.CROSS_PROCESS_ATTRIBUTES)
+        self._store_attributes()
+
+    def _store_attributes(self):
+        """"
+        Store all values from the attributes from run_check.CROSS_PROCESS_ATTRIBUTES on this object,
+        in case multiprocessing is using spawn as its starting method. 
+        """
+
+        # Attributes only need to be passed explicitely to child processes when using spawn
+        if multiprocessing.get_start_method() != "spawn":
+           return
+
+        self._attribute_values = [eval(name) for name in self.CROSS_PROCESS_ATTRIBUTES]
+
+        # Replace all unpickle-able values with nothing, assuming they've been set externally,
+        # and will be set again upon re-importing the checks module
+        # https://github.com/cs50/check50/issues/235
+        for i, value in enumerate(self._attribute_values):
+            try:
+                pickle.dumps(value)
+            except (pickle.PicklingError, AttributeError):
+                self._attribute_values[i] = None
+
+        self._attribute_values = tuple(self._attribute_values)
+
+
+    def _set_attributes(self):
+        """
+        If the parent process set any values in self._attribute_values, 
+        restore them in the child process.
+        """
+        if not hasattr(self, "_attribute_values"):
+           return
+
+        for name, val in zip(self.CROSS_PROCESS_ATTRIBUTES, self._attribute_values):
+            self._set_attribute(name, val)
+
 
     @staticmethod
     def _set_attribute(name, value):
@@ -351,12 +389,18 @@ def _set_attribute(name, value):
 
         setattr(obj, parts[-1], value)
 
+
     def __call__(self):
-        for name, val in zip(self.CROSS_PROCESS_ATTRIBUTES, self.attribute_values):
-            self._set_attribute(name, val)
+        # Restore any attributes from the parent process
+        self._set_attributes()
 
+        # Create the checks module
         mod = importlib.util.module_from_spec(self.spec)
+
+        # Execute (effectively import) the checks module
         self.spec.loader.exec_module(mod)
+
+        # Run just the check named self.check_name
         internal.check_running = True
         try:
             return getattr(mod, self.check_name)(internal.run_root_dir, self.state)

setup.py

@@ -29,6 +29,6 @@
         "console_scripts": ["check50=check50.__main__:main"]
     },
     url="https://github.com/cs50/check50",
-    version="3.2.0",
+    version="3.2.1",
     include_package_data=True
 )

tests/checks/unpicklable_attribute/.cs50.yaml

@@ -0,0 +1 @@
+check50: true

tests/checks/unpicklable_attribute/__init__.py

@@ -0,0 +1,9 @@
+import check50
+import sys
+
+# Set the excepthook to an unpicklable value, the run_check process will try to copy this
+sys.excepthook = lambda type, value, traceback: "bar"
+
+@check50.check()
+def foo():
+    """foo"""

tests/runner_tests.py

@@ -0,0 +1,70 @@
+import check50
+import check50.runner
+
+import importlib
+import multiprocessing
+import os
+import pathlib
+import pexpect
+import sys
+import tempfile
+import unittest
+
+
+CHECKS_DIRECTORY = pathlib.Path(__file__).absolute().parent / "checks"
+CHECK50_SUPPORTED_START_METHODS = ("fork", "spawn")
+
+
+# Just test spawn under OS X due to a bug with "fork": https://bugs.python.org/issue33725
+if sys.platform == "darwin":
+    SUPPORTED_START_METHODS = ("spawn",)
+
+# Don't test forkserver under linux, serves no usecase for check50
+else:
+    SUPPORTED_START_METHODS = tuple(set(CHECK50_SUPPORTED_START_METHODS) & set(multiprocessing.get_all_start_methods()))
+
+
+class TestMultiprocessingStartMethods(unittest.TestCase):
+    def setUp(self):
+        self.working_directory = tempfile.TemporaryDirectory()
+        os.chdir(self.working_directory.name)
+
+        # Keep track of get_start_method
+        # This function gets monkey patched to ensure run_check is aware of the multiprocessing context, 
+        # without needing to explicitely pass the context to run_check.
+        # The same behavior can't be achieved by multiprocessing.set_start_method as that can only run once per program
+        # https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods
+        self._get_start_method = multiprocessing.get_start_method()
+
+    def tearDown(self):
+        self.working_directory.cleanup()
+        multiprocessing.get_start_method = self._get_start_method
+
+    def test_unpicklable_attribute(self):
+        # Create the checks_spec and check_name needed for run_check
+        checks_path = CHECKS_DIRECTORY / "unpicklable_attribute/__init__.py"
+        check_name = "foo"
+        spec = importlib.util.spec_from_file_location("checks", checks_path)
+
+        # Execute the module once in the main process, as the Runner does too
+        # This will set sys.excepthook to an unpicklable lambda
+        check_module = importlib.util.module_from_spec(spec)
+        spec.loader.exec_module(check_module)
+
+        # For each available method
+        for start_method in SUPPORTED_START_METHODS:
+
+            # Create a multiprocessing context for that method
+            ctx = multiprocessing.get_context(start_method)
+
+            # Monkey patch get_start_method() used by run_check to check for its method
+            multiprocessing.get_start_method = lambda: start_method
+
+            # Start and join each check process
+            p = ctx.Process(target=check50.runner.run_check(check_name, spec))
+            p.start()
+            p.join()
+
+
+if __name__ == "__main__":
+    unittest.main()