classification
Title: Remove assertion-based checking in multiprocessing
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Winterflower, berker.peksag, drallensmith, jesstess, jnoller, pitrou, sbt, vladris
Priority: normal Keywords: patch

Created on 2009-01-19 16:32 by jnoller, last changed 2017-08-19 13:27 by drallensmith.

Files
File name Uploaded Description Edit
issue5001.diff vladris, 2011-10-02 20:43 Refactored asserts review
issue5001_v2.diff vladris, 2011-10-02 23:37 Second iteration review
Pull Requests
URL Status Linked Edit
PR 3078 merged drallensmith, 2017-08-12 12:45
PR 3079 open drallensmith, 2017-08-12 18:59
Messages (15)
msg80192 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2009-01-19 16:32
Right now, the multiprocessing code is littered with statements like:

        assert self._popen is None, 'cannot start a process twice'
        assert self._parent_pid == os.getpid(), \
               'can only start a process object created by current 
process'
        assert not _current_process._daemonic, \
               'daemonic processes are not allowed to have children'

These will obviously be stripped out if running in optimized mode - 
however its not cool to rely on these anyway, the code should be 
refactored to proper checks, e.g.:

    if not hasattr(lock, 'acquire'):
        raise AttributeError("'%r' has no method 'acquire'" % lock)
msg112140 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-07-31 11:39
@Jesse: could you provide a patch that addresses this issue?
msg112149 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2010-07-31 13:07
@Mark Yeah - I'm the current multiprocessing maintainer. If I fix it, I'll just commit it :) I filed this as a to do against myself.
msg144780 - (view) Author: Vlad Riscutia (vladris) Date: 2011-10-02 20:43
I attached a patch which replaces all asserts with checks that raise exceptions. I used my judgement in determining exception types but I might have been off in some places. Also, this patch replaces ALL asserts. It is possible that some of the internal functions should be kept using asserts but I am not familiar enough with the module to say which. I figured I'd better do the whole thing than reviewers can say where lines should be reverted to asserts.
msg144781 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-10-02 21:26
Thank you. I've attached some comments, click on the "review" link to read them.
msg144795 - (view) Author: Vlad Riscutia (vladris) Date: 2011-10-02 23:37
Thanks for the quick review! I attached second iteration addressing feedback + changed all occurrences of checks like "type(x) is y" to "isinstance(x, y)".

I would appreciate a second look because this patch has many small changes and even though I ran full test suit which passed, I'm afraid I made a typo somewhere :)
msg217299 - (view) Author: Jessica McKellar (jesstess) * Date: 2014-04-27 17:54
Thanks for the patches, vladris!

I've reviewed the latest version, and it addresses all of Antoine's review feedback. Ezio left some additional feedback (http://bugs.python.org/review/5001/#ps3407) which still needs to be addressed.
msg300195 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-08-12 10:42
@drallensmith, the patch you commented on is very old and probably doesn't apply anymore.  It would be useful to make an updated patch (or rather PR, as we are using Github for development now: see https://devguide.python.org/pullrequest/).
msg300196 - (view) Author: (drallensmith) * Date: 2017-08-12 11:41
Patches (for 2.7 and 3.5; the code is identical, only the line numbers changed) for the portion I commented on can be found in issue 31169.
msg300201 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-08-12 15:37
New changeset 48d9823a0ebde4dfab8bc154bb6df462fb2ee403 by Antoine Pitrou (Allen W. Smith, Ph.D) in branch 'master':
bpo-5001, bpo-31169: Fix two uninformative asserts in multiprocessing/managers.py (#3078)
https://github.com/python/cpython/commit/48d9823a0ebde4dfab8bc154bb6df462fb2ee403
msg300397 - (view) Author: (drallensmith) * Date: 2017-08-17 00:22
Pull request submitted (4 days ago) for managers.py, queue.py; just added pool.py changes to those.
msg300473 - (view) Author: (drallensmith) * Date: 2017-08-18 03:52
No discussion yet on pull request (5 days); just submitted fixes for util.py.
msg300512 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-08-18 15:56
Please allow us a bit of time :-)  We are all volunteers here.
msg300541 - (view) Author: (drallensmith) * Date: 2017-08-18 22:19
Sorry! I was starting to wonder if the PR had been overlooked somehow...
msg300584 - (view) Author: (drallensmith) * Date: 2017-08-19 13:27
I've updated the PR to include all of the non-Windows-specific asserts; I am not sufficiently familiar with Windows multiprocessing to feel confident writing informative error messages.
History
Date User Action Args
2017-08-19 13:27:04drallensmithsetmessages: + msg300584
2017-08-18 22:19:15drallensmithsetmessages: + msg300541
2017-08-18 15:56:13pitrousetmessages: + msg300512
2017-08-18 03:52:15drallensmithsetmessages: + msg300473
2017-08-17 00:22:13drallensmithsetmessages: + msg300397
versions: + Python 3.7, - Python 3.6
2017-08-12 18:59:13drallensmithsetpull_requests: + pull_request3120
2017-08-12 15:37:11pitrousetmessages: + msg300201
2017-08-12 12:45:07drallensmithsetpull_requests: + pull_request3117
2017-08-12 11:41:49drallensmithsetmessages: + msg300196
2017-08-12 10:42:29pitrousetmessages: + msg300195
2017-08-10 01:34:05drallensmithsetnosy: + drallensmith
2017-02-12 20:33:20Winterflowersetnosy: + Winterflower
2016-01-21 08:42:57berker.peksagsetnosy: + berker.peksag

type: enhancement
versions: + Python 3.6, - Python 2.7, Python 3.4, Python 3.5
2014-04-28 00:15:49ned.deilysetnosy: + sbt

versions: + Python 3.4, Python 3.5, - Python 3.2, Python 3.3
2014-04-27 17:54:56jesstesssetnosy: + jesstess
messages: + msg217299
2011-10-02 23:37:30vladrissetfiles: + issue5001_v2.diff

messages: + msg144795
2011-10-02 21:26:26pitrousetversions: + Python 3.3, - Python 3.1
nosy: + pitrou, - BreamoreBoy
messages: + msg144781

assignee: jnoller ->
stage: needs patch -> patch review
2011-10-02 20:43:49vladrissetfiles: + issue5001.diff

nosy: + vladris
messages: + msg144780

keywords: + patch
2010-07-31 13:07:06jnollersetmessages: + msg112149
2010-07-31 11:39:07BreamoreBoysetversions: + Python 3.2
nosy: + BreamoreBoy

messages: + msg112140

stage: needs patch
2009-01-22 19:22:11jnollerlinkissue3273 superseder
2009-01-19 16:32:42jnollercreate