BuildBackendHookCaller.subprocess_runner() context manager is not thread-safe

[gaborbernat]

Description

BuildBackendHookCaller.subprocess_runner() context manager mutates self._subprocess_runner on the instance without any synchronization:

@contextmanager
def subprocess_runner(self, runner):
    prev = self._subprocess_runner
    self._subprocess_runner = runner
    try:
        yield
    finally:
        self._subprocess_runner = prev

If two threads share the same BuildBackendHookCaller instance and one of them uses subprocess_runner() to temporarily swap the runner, the other thread sees the swapped runner and may invoke hooks with the wrong runner. The restore in finally also races with concurrent reads of self._subprocess_runner in _call_hook.

Impact

This prevents consumers (e.g. pypa/build) from safely sharing a BuildBackendHookCaller (or a wrapping ProjectBuilder) across threads when subprocess_runner() is used.

Discovered during a thread-safety audit of pypa/build for pypa/build#377.

[takluyver]

BuildBackendHookCaller should be pretty cheap to create. Is it reasonable to document that multi-threaded callers should create a separate instance per thread, rather than adding something like a thread local? Or even just document that you shouldn't use the convenience context manager along with threads? You can set the context manager subprocess runner when instantiating the object.

FWIW, I generally don't assume that arbitrary Python objects are thread safe unless it's documented as such.