bpo-5001, bpo-31169: Fix two uninformative asserts in multiprocessing/managers.py#3078
Conversation
Replace assertions in error-reporting code with more-informative version that doesn't cause confusion over where and what the error is.
| return RemoteError('Unserializable message: %s\n' % result) | ||
| elif kind in ('#TRACEBACK', '#UNSERIALIZABLE'): | ||
| if not isinstance(result, str): | ||
| raise SystemError( |
There was a problem hiding this comment.
We usually reserve SystemError for low-level errors inside the interpreter, so I suggest using something else.
There was a problem hiding this comment.
Ah; I had thought it would include low-level inside standard library files, if it was something that did not appear likely to be from a user mistake. Thanks; changing to TypeError, unless there is a better one.
|
Thanks for the PR. Apart from the comment I added, it also needs a NEWS entry as added with the "blurb" utility. |
As suggested in PR comment by @pitrou, changing from SystemError; TypeError appears appropriate.
|
@pitrou - would it work to do a |
|
Quite welcome, BTW. |
Probably? I don't use a separate clone for "blurb" myself, so I'm not sure why you would need one. |
|
@pitrou - I currently don't have a local clone of cpython due to disk space issues; I am thinking I could manage a shallow clone of my fork, which would also enable me to (long-term) work on some of the other asserts, expand the code change to other dev versions, etc. |
|
Yes, a shallow clone would probably work, at least I don't see a reason against it. |
…by additional work)
|
NEWS file added as per helpful instruction from @pitrou - also ACKS, although not sure if I've really justified it yet... will work on doing so. |
|
Any contributor deserves to be in Misc/ACKS, no worries here :-) |
|
Thank you for contributing this @drallensmith ! |
4 participants
There are quite a few uninformative asserts in the multiprocessing code, as discussed in issue5001 and issue31169. One of them that is most potentially problematic is in error-reporting code, in convert_to_error. (I will try to work on some of the other asserts when I have the time, but I am running low on disk space and suspect a clone of the entire cpython repository would be rather large...) For this PR, the identical code is present in 2.7.13 and 3.5.3, and I strongly suspect in all other versions between 2.7 and 3.7.
https://bugs.python.org/issue5001