This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9660

worker: POSIX only supports workers from main thread (issue6460)
ClosedPublic

Authored by joerg.sonnenberger on Dec 27 2020, 7:05 PM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHGa42502e9ae6d: worker: POSIX only supports workers from main thread (issue6460)
Summary

The POSIX backend sets signal handlers for SIGINT (maybe avoidable) and
SIGCHLD (necessary for waitpid). Python up to 3.9 only allow this from
the main thread, so disable the worker feature otherwise.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

joerg.sonnenberger added a comment.Dec 29 2020, 3:07 PM

This is a bit of a proof-of-concept. The check is likely stricter than necessary, i.e. I didn't check what the constraints are for Windows.

marmoute requested changes to this revision.Jan 7 2021, 8:14 AM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
mercurial/worker.py
157

This seems like a step in the right direction (if I understand the issue correctly). However this kind of "arbitrary" restriction warrant a clear comment block to explain why we do it.

This revision now requires changes to proceed.Jan 7 2021, 8:14 AM
marmoute accepted this revision.Jan 7 2021, 8:28 AM

Thanks!

joerg.sonnenberger retitled this revision from worker: restrict use of worker procesess to the main thread to worker: POSIX only supports workers from main thread (issue6460).Jan 11 2021, 10:42 AM
joerg.sonnenberger edited the summary of this revision. (Show Details)
joerg.sonnenberger added a commit: rHGa42502e9ae6d: worker: POSIX only supports workers from main thread (issue6460).Jan 13 2021, 2:40 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGa42502e9ae6d: worker: POSIX only supports workers from main thread (issue6460) (authored by joerg.sonnenberger). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/worker.py (11 lines)

Diff 24799

mercurial/worker.py

return n return n
except ValueError: except ValueError:
raise error.Abort(_(b'number of cpus must be an integer')) raise error.Abort(_(b'number of cpus must be an integer'))
return min(max(countcpus(), 4), 32) return min(max(countcpus(), 4), 32)
if pycompat.ispy3: if pycompat.ispy3:
def ismainthread():
return threading.current_thread() == threading.main_thread()
class _blockingreader(object): class _blockingreader(object):
def __init__(self, wrapped): def __init__(self, wrapped):
self._wrapped = wrapped self._wrapped = wrapped
# Do NOT implement readinto() by making it delegate to # Do NOT implement readinto() by making it delegate to
# _wrapped.readinto(), since that is unbuffered. The unpickler is fine # _wrapped.readinto(), since that is unbuffered. The unpickler is fine
# with just read() and readline(), so we don't need to implement it. # with just read() and readline(), so we don't need to implement it.
del view del view
del buf[pos:] del buf[pos:]
return bytes(buf) return bytes(buf)
else: else:
def ismainthread():
return isinstance(threading.current_thread(), threading._MainThread)
def _blockingreader(wrapped): def _blockingreader(wrapped):
return wrapped return wrapped
if pycompat.isposix or pycompat.iswindows: if pycompat.isposix or pycompat.iswindows:
_STARTUP_COST = 0.01 _STARTUP_COST = 0.01
# The Windows worker is thread based. If tasks are CPU bound, threads # The Windows worker is thread based. If tasks are CPU bound, threads
# in the presence of the GIL result in excessive context switching and # in the presence of the GIL result in excessive context switching and
iterator then a dict (encoded as an iterator that yield many (False, ..) iterator then a dict (encoded as an iterator that yield many (False, ..)
then a (True, dict)). The dicts are joined in some arbitrary order, so then a (True, dict)). The dicts are joined in some arbitrary order, so
overlapping keys are a bad idea. overlapping keys are a bad idea.
threadsafe - whether work items are thread safe and can be executed using threadsafe - whether work items are thread safe and can be executed using
a thread-based worker. Should be disabled for CPU heavy tasks that don't a thread-based worker. Should be disabled for CPU heavy tasks that don't
release the GIL. release the GIL.
""" """
enabled = ui.configbool(b'worker', b'enabled') enabled = ui.configbool(b'worker', b'enabled')
marmouteUnsubmitted
Not Done

This seems like a step in the right direction (if I understand the issue correctly). However this kind of "arbitrary" restriction warrant a clear comment block to explain why we do it.

marmoute: This seems like a step in the right direction (if I understand the issue correctly). However…
if enabled and _platformworker is _posixworker and not ismainthread():
# The POSIX worker has to install a handler for SIGCHLD.
# Python up to 3.9 only allows this in the main thread.
enabled = False
if enabled and worthwhile(ui, costperarg, len(args), threadsafe=threadsafe): if enabled and worthwhile(ui, costperarg, len(args), threadsafe=threadsafe):
return _platformworker(ui, func, staticargs, args, hasretval) return _platformworker(ui, func, staticargs, args, hasretval)
return func(*staticargs + (args,)) return func(*staticargs + (args,))
def _posixworker(ui, func, staticargs, args, hasretval): def _posixworker(ui, func, staticargs, args, hasretval):
workers = _numworkers(ui) workers = _numworkers(ui)
oldhandler = signal.getsignal(signal.SIGINT) oldhandler = signal.getsignal(signal.SIGINT)