Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
23f0559
Make error message more informative
drallensmith Aug 12, 2017
09e2391
Additional clarification + get travis to check
drallensmith Aug 12, 2017
e8b45b6
Change from SystemError to TypeError
drallensmith Aug 12, 2017
1fa907e
NEWS file installation; ACKS addition (will do my best to justify it …
drallensmith Aug 12, 2017
6344adc
Merge branch 'master' of https://github.com/python/cpython - avoid re…
drallensmith Aug 12, 2017
3251312
Making current AssertionErrors in multiprocessing more informative
drallensmith Aug 12, 2017
cfae761
Blurb added re multiprocessing managers.py, queues.py cleanup
drallensmith Aug 12, 2017
c0fb430
Further multiprocessing cleanup - went through pool.py
drallensmith Aug 17, 2017
fa446ae
Fix two asserts in multiprocessing/util.py
drallensmith Aug 18, 2017
f90e227
Most asserts in multiprocessing more informative
drallensmith Aug 18, 2017
8220864
Didn't save right version
drallensmith Aug 18, 2017
70cd790
Further work on multiprocessing error messages
drallensmith Aug 28, 2017
ae7a107
Correct typo
drallensmith Aug 28, 2017
f29c97f
Correct typo v2
drallensmith Aug 28, 2017
fb3fb93
Blasted colon... serves me right for trying to work on two things at …
drallensmith Aug 28, 2017
58a5a8e
Simplify NEWS entry
pitrou Aug 29, 2017
4bda162
Update 2017-08-18-17-16-38.bpo-5001.gwnthq.rst
drallensmith Aug 29, 2017
04f0a44
Update 2017-08-18-17-16-38.bpo-5001.gwnthq.rst
drallensmith Aug 29, 2017
c96980d
Corrected (thanks to pitrou) error messages for notify
drallensmith Aug 29, 2017
01f21fd
Merge branch 'master' of https://github.com/drallensmith/cpython - ma…
drallensmith Aug 29, 2017
4a7558b
Remove extraneous backslash in docstring.
pitrou Aug 29, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions Lib/multiprocessing/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,9 @@ def PipeClient(address):

def deliver_challenge(connection, authkey):
import hmac
assert isinstance(authkey, bytes)
if not isinstance(authkey, bytes):
raise ValueError(
"Authkey must be bytes, not {0!s}".format(type(authkey)))
message = os.urandom(MESSAGE_LENGTH)
connection.send_bytes(CHALLENGE + message)
digest = hmac.new(authkey, message, 'md5').digest()
Expand All @@ -733,7 +735,9 @@ def deliver_challenge(connection, authkey):

def answer_challenge(connection, authkey):
import hmac
assert isinstance(authkey, bytes)
if not isinstance(authkey, bytes):
raise ValueError(
"Authkey must be bytes, not {0!s}".format(type(authkey)))
message = connection.recv_bytes(256) # reject large message
assert message[:len(CHALLENGE)] == CHALLENGE, 'message = %r' % message
message = message[len(CHALLENGE):]
Expand Down
5 changes: 4 additions & 1 deletion Lib/multiprocessing/dummy/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ def __init__(self, group=None, target=None, name=None, args=(), kwargs={}):
self._parent = current_process()

def start(self):
assert self._parent is current_process()
if self._parent is not current_process():
raise RuntimeError(
"Parent is {0!r} but current_process is {1!r}".format(
self._parent, current_process()))
self._start_called = True
if hasattr(self._parent, '_children'):
self._parent._children[self] = None
Expand Down
12 changes: 9 additions & 3 deletions Lib/multiprocessing/forkserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ def sigchld_handler(*_unused):

if alive_r in rfds:
# EOF because no more client processes left
assert os.read(alive_r, 1) == b''
assert os.read(alive_r, 1) == b'', "Not at EOF?"
raise SystemExit

if sig_r in rfds:
Expand All @@ -208,7 +208,10 @@ def sigchld_handler(*_unused):
if os.WIFSIGNALED(sts):
returncode = -os.WTERMSIG(sts)
else:
assert os.WIFEXITED(sts)
if not os.WIFEXITED(sts):
raise AssertionError(
"Child {0:n} status is {1:n}".format(
pid,sts))
returncode = os.WEXITSTATUS(sts)
# Send exit code to client process
try:
Expand All @@ -227,7 +230,10 @@ def sigchld_handler(*_unused):
with listener.accept()[0] as s:
# Receive fds from client
fds = reduction.recvfds(s, MAXFDS_TO_SEND + 1)
assert len(fds) <= MAXFDS_TO_SEND
if len(fds) > MAXFDS_TO_SEND:
raise RuntimeError(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this one is necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will get back to you on this one - need to put together lecture for tomorrow...

@drallensmith drallensmith Aug 29, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would it not be necessary? Also see below.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because otherwise would probably a bug in the underlying recvmsg implementation, which I think doesn't need to be guarded by a nice error message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually more worried about detecting what bug is happening - see here for why, noting that it's with a nightly build thus has the first patch in it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, fair enough.

"Too many ({0:n}) fds to send".format(
len(fds)))
child_r, child_w, *fds = fds
s.close()
pid = os.fork()
Expand Down
15 changes: 12 additions & 3 deletions Lib/multiprocessing/heap.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,10 @@ def free(self, block):
# synchronously sometimes later from malloc() or free(), by calling
# _free_pending_blocks() (appending and retrieving from a list is not
# strictly thread-safe but under cPython it's atomic thanks to the GIL).
assert os.getpid() == self._lastpid
if os.getpid() != self._lastpid:
raise ValueError(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto to the above.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ow. I should have been more explicit because now I don't remember what I meant here. Sorry :-(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an absent-minded professor, I understand fully :-}

"My pid ({0:n}) is not last pid {1:n}".format(
os.getpid(),self._lastpid))
if not self._lock.acquire(False):
# can't acquire the lock right now, add the block to the list of
# pending blocks to free
Expand All @@ -227,7 +230,10 @@ def free(self, block):

def malloc(self, size):
# return a block of right size (possibly rounded up)
assert 0 <= size < sys.maxsize
if size < 0:
raise ValueError("Size {0:n} out of range".format(size))
if sys.maxsize <= size:
raise OverflowError("Size {0:n} too large".format(size))
if os.getpid() != self._lastpid:
self.__init__() # reinitialize after fork
with self._lock:
Expand All @@ -250,7 +256,10 @@ class BufferWrapper(object):
_heap = Heap()

def __init__(self, size):
assert 0 <= size < sys.maxsize
if size < 0:
raise ValueError("Size {0:n} out of range".format(size))
if sys.maxsize <= size:
raise OverflowError("Size {0:n} too large".format(size))
block = BufferWrapper._heap.malloc(size)
self._state = (block, size)
util.Finalize(self, BufferWrapper._heap.free, args=(block,))
Expand Down
54 changes: 44 additions & 10 deletions Lib/multiprocessing/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from traceback import format_exc

from . import connection
from .context import reduction, get_spawning_popen
from .context import reduction, get_spawning_popen, ProcessError
from . import pool
from . import process
from . import util
Expand Down Expand Up @@ -133,7 +133,10 @@ class Server(object):
'debug_info', 'number_of_objects', 'dummy', 'incref', 'decref']

def __init__(self, registry, address, authkey, serializer):
assert isinstance(authkey, bytes)
if not isinstance(authkey, bytes):
raise TypeError(
"Authkey {0!r} is type {1!s}, not bytes".format(
authkey, type(authkey)))
self.registry = registry
self.authkey = process.AuthenticationString(authkey)
Listener, Client = listener_client[serializer]
Expand Down Expand Up @@ -163,7 +166,7 @@ def serve_forever(self):
except (KeyboardInterrupt, SystemExit):
pass
finally:
if sys.stdout != sys.__stdout__:
if sys.stdout != sys.__stdout__: # what about stderr?
util.debug('resetting stdout, stderr')
sys.stdout = sys.__stdout__
sys.stderr = sys.__stderr__
Expand Down Expand Up @@ -316,6 +319,7 @@ def debug_info(self, c):
'''
Return some info --- useful to spot problems with refcounting
'''
# Perhaps include debug info about 'c'?
with self.mutex:
result = []
keys = list(self.id_to_refcount.keys())
Expand Down Expand Up @@ -356,15 +360,20 @@ def create(self, c, typeid, *args, **kwds):
self.registry[typeid]

if callable is None:
assert len(args) == 1 and not kwds
if kwds or (len(args) != 1):
raise ValueError(
"Without callable, must have one non-keyword argument")
obj = args[0]
else:
obj = callable(*args, **kwds)

if exposed is None:
exposed = public_methods(obj)
if method_to_typeid is not None:
assert type(method_to_typeid) is dict
if not isinstance(method_to_typeid, dict):
raise TypeError(
"Method_to_typeid {0!r}: type {1!s}, not dict".format(
method_to_typeid, type(method_to_typeid)))
exposed = list(exposed) + list(method_to_typeid)

ident = '%x' % id(obj) # convert to string because xmlrpclib
Expand Down Expand Up @@ -417,7 +426,11 @@ def decref(self, c, ident):
return

with self.mutex:
assert self.id_to_refcount[ident] >= 1
if self.id_to_refcount[ident] <= 0:
raise AssertionError(
"Id {0!s} ({1!r}) has refcount {2:n}, not 1+".format(
ident, self.id_to_obj[ident],
self.id_to_refcount[ident]))
self.id_to_refcount[ident] -= 1
if self.id_to_refcount[ident] == 0:
del self.id_to_refcount[ident]
Expand Down Expand Up @@ -480,7 +493,14 @@ def get_server(self):
'''
Return server object with serve_forever() method and address attribute
'''
assert self._state.value == State.INITIAL
if self._state.value != State.INITIAL:
if self._state.value == State.STARTED:
raise ProcessError("Already started server")
elif self._state.value == State.SHUTDOWN:
raise ProcessError("Manager has shut down")
else:
raise ProcessError(
"Unknown state {!r}".format(self._state.value))
return Server(self._registry, self._address,
self._authkey, self._serializer)

Expand All @@ -497,7 +517,14 @@ def start(self, initializer=None, initargs=()):
'''
Spawn a server process for this manager object
'''
assert self._state.value == State.INITIAL
if self._state.value != State.INITIAL:
if self._state.value == State.STARTED:
raise ProcessError("Already started server")
elif self._state.value == State.SHUTDOWN:
raise ProcessError("Manager has shut down")
else:
raise ProcessError(
"Unknown state {!r}".format(self._state.value))

if initializer is not None and not callable(initializer):
raise TypeError('initializer must be a callable')
Expand Down Expand Up @@ -593,7 +620,14 @@ def _number_of_objects(self):
def __enter__(self):
if self._state.value == State.INITIAL:
self.start()
assert self._state.value == State.STARTED
if self._state.value != State.STARTED:
if self._state.value == State.INITIAL:
raise ProcessError("Unable to start server")
elif self._state.value == State.SHUTDOWN:
raise ProcessError("Manager has shut down")
else:
raise ProcessError(
"Unknown state {!r}".format(self._state.value))
return self

def __exit__(self, exc_type, exc_val, exc_tb):
Expand Down Expand Up @@ -653,7 +687,7 @@ def register(cls, typeid, callable=None, proxytype=None, exposed=None,
getattr(proxytype, '_method_to_typeid_', None)

if method_to_typeid:
for key, value in list(method_to_typeid.items()):
for key, value in list(method_to_typeid.items()): # isinstance?
assert type(key) is str, '%r is not a string' % key
assert type(value) is str, '%r is not a string' % value

Expand Down
28 changes: 21 additions & 7 deletions Lib/multiprocessing/pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ def __repr__(self):

def worker(inqueue, outqueue, initializer=None, initargs=(), maxtasks=None,
wrap_exception=False):
assert maxtasks is None or (type(maxtasks) == int and maxtasks > 0)
if (maxtasks is not None) and not (isinstance(maxtasks, int)
and maxtasks >= 1):
raise AssertionError("Maxtasks {!r} is not valid".format(maxtasks))
put = outqueue.put
get = inqueue.get
if hasattr(inqueue, '_writer'):
Expand Down Expand Up @@ -254,8 +256,8 @@ def _setup_queues(self):
def apply(self, func, args=(), kwds={}):
'''
Equivalent of `func(*args, **kwds)`.
Pool must be running.
'''
assert self._state == RUN
return self.apply_async(func, args, kwds).get()

def map(self, func, iterable, chunksize=None):
Expand Down Expand Up @@ -307,6 +309,10 @@ def imap(self, func, iterable, chunksize=1):
))
return result
else:
if chunksize < 1:
raise ValueError(
"Chunksize must be 1+, not {0:n}".format(
chunksize))
assert chunksize > 1
task_batches = Pool._get_tasks(func, iterable, chunksize)
result = IMapIterator(self._cache)
Expand Down Expand Up @@ -334,7 +340,9 @@ def imap_unordered(self, func, iterable, chunksize=1):
))
return result
else:
assert chunksize > 1
if chunksize < 1:
raise ValueError(
"Chunksize must be 1+, not {0!r}".format(chunksize))
task_batches = Pool._get_tasks(func, iterable, chunksize)
result = IMapUnorderedIterator(self._cache)
self._taskqueue.put(
Expand Down Expand Up @@ -466,7 +474,7 @@ def _handle_results(outqueue, get, cache):
return

if thread._state:
assert thread._state == TERMINATE
assert thread._state == TERMINATE, "Thread not in TERMINATE"
util.debug('result handler found thread._state=TERMINATE')
break

Expand Down Expand Up @@ -542,7 +550,10 @@ def terminate(self):

def join(self):
util.debug('joining pool')
assert self._state in (CLOSE, TERMINATE)
if self._state == RUN:
raise ValueError("Pool is still running")
elif self._state not in (CLOSE, TERMINATE):
raise ValueError("In unknown state")
self._worker_handler.join()
self._task_handler.join()
self._result_handler.join()
Expand Down Expand Up @@ -570,7 +581,9 @@ def _terminate_pool(cls, taskqueue, inqueue, outqueue, pool,
util.debug('helping task handler/workers to finish')
cls._help_stuff_finish(inqueue, task_handler, len(pool))

assert result_handler.is_alive() or len(cache) == 0
if (not result_handler.is_alive()) and (len(cache) != 0):
raise AssertionError(
"Cannot have cache with result_hander not alive")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get the point of adding a message here. The message will not be understandable for the average user, and whoever wants to debug it has to read the source code anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does one tell where the AssertionError is happening, if the pool is being used not by the primary process but by a subprocess (possibly on another machine via a manager)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to the line number in the traceback, I guess?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish that worked in the circumstances I noted above - see here, noting that it's a "nightly" build so includes the earlier patch, for an example.


result_handler._state = TERMINATE
outqueue.put(None) # sentinel
Expand Down Expand Up @@ -628,7 +641,8 @@ def ready(self):
return self._event.is_set()

def successful(self):
assert self.ready()
if not self.ready():
raise ValueError("{0!r} not ready".format(self))
return self._success

def wait(self, timeout=None):
Expand Down
2 changes: 1 addition & 1 deletion Lib/multiprocessing/popen_fork.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def poll(self, flag=os.WNOHANG):
if os.WIFSIGNALED(sts):
self.returncode = -os.WTERMSIG(sts)
else:
assert os.WIFEXITED(sts)
assert os.WIFEXITED(sts), "Status is {:n}".format(sts)
self.returncode = os.WEXITSTATUS(sts)
return self.returncode

Expand Down
6 changes: 3 additions & 3 deletions Lib/multiprocessing/queues.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def _after_fork(self):
self._poll = self._reader.poll

def put(self, obj, block=True, timeout=None):
assert not self._closed
assert not self._closed, "Queue {0!r} has been closed".format(self)
if not self._sem.acquire(block, timeout):
raise Full

Expand Down Expand Up @@ -140,7 +140,7 @@ def close(self):

def join_thread(self):
debug('Queue.join_thread()')
assert self._closed
assert self._closed, "Queue {0!r} not closed".format(self)
if self._jointhread:
self._jointhread()

Expand Down Expand Up @@ -281,7 +281,7 @@ def __setstate__(self, state):
self._cond, self._unfinished_tasks = state[-2:]

def put(self, obj, block=True, timeout=None):
assert not self._closed
assert not self._closed, "Queue {0!r} is closed".format(self)
if not self._sem.acquire(block, timeout):
raise Full

Expand Down
5 changes: 4 additions & 1 deletion Lib/multiprocessing/reduction.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,10 @@ def recvfds(sock, size):
if len(cmsg_data) % a.itemsize != 0:
raise ValueError
a.frombytes(cmsg_data)
assert len(a) % 256 == msg[0]
if len(a) % 256 != msg[0]:
raise AssertionError(
"Len is {0:n} but msg[0] is {1!r}".format(
len(a), msg[0]))
return list(a)
except (ValueError, IndexError):
pass
Expand Down
2 changes: 1 addition & 1 deletion Lib/multiprocessing/resource_sharer.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def _afterfork(self):

def _start(self):
from .connection import Listener
assert self._listener is None
assert self._listener is None, "Already have Listener"
util.debug('starting listener and thread for sending handles')
self._listener = Listener(authkey=process.current_process().authkey)
self._address = self._listener.address
Expand Down
3 changes: 2 additions & 1 deletion Lib/multiprocessing/semaphore_tracker.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ def _send(self, cmd, name):
# bytes are atomic, and that PIPE_BUF >= 512
raise ValueError('name too long')
nbytes = os.write(self._fd, msg)
assert nbytes == len(msg)
assert nbytes == len(msg), "nbytes {0:n} but len(msg) {1:n}".format(
nbytes, len(msg))


_semaphore_tracker = SemaphoreTracker()
Expand Down
2 changes: 1 addition & 1 deletion Lib/multiprocessing/spawn.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def spawn_main(pipe_handle, parent_pid=None, tracker_fd=None):
'''
Run code specified by data received over pipe
'''
assert is_forking(sys.argv)
assert is_forking(sys.argv), "Not forking"
if sys.platform == 'win32':
import msvcrt
new_handle = reduction.steal_handle(parent_pid, pipe_handle)
Expand Down
Loading