New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove assertion-based checking in multiprocessing #49251
Comments
Right now, the multiprocessing code is littered with statements like:
process' These will obviously be stripped out if running in optimized mode - if not hasattr(lock, 'acquire'):
raise AttributeError("'%r' has no method 'acquire'" % lock) |
@jesse: could you provide a patch that addresses this issue? |
@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. |
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. |
Thank you. I've attached some comments, click on the "review" link to read them. |
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 :) |
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. |
@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/). |
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 bpo-31169. |
Pull request submitted (4 days ago) for managers.py, queue.py; just added pool.py changes to those. |
No discussion yet on pull request (5 days); just submitted fixes for util.py. |
Please allow us a bit of time :-) We are all volunteers here. |
Sorry! I was starting to wonder if the PR had been overlooked somehow... |
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. |
I suppose this can be closed now. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: