classification
Title: Error when unpickling IncompleteReadError
Type: Stage: resolved
Components: asyncio Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: badouxn, serhiy.storchaka, yselivanov
Priority: normal Keywords: patch

Created on 2017-11-15 12:53 by badouxn, last changed 2017-11-16 00:28 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
Impossible_to_unpickle_IncompleteReadError.py badouxn, 2017-11-15 12:53 python script highlighting the issue
Pull Requests
URL Status Linked Edit
PR 4404 closed badouxn, 2017-11-15 13:12
PR 4409 merged yselivanov, 2017-11-15 18:46
PR 4411 merged python-dev, 2017-11-15 22:14
Messages (8)
msg306268 - (view) Author: (badouxn) * Date: 2017-11-15 12:53
When using asyncio in combination with multiprocessing, a TypeError occur when readuntil() encounter an EOF instead of the delimiter.
readuntil return a IncompleteReadError exception which is pickled by the multiprocessing package.
The unpickling failed due to an argument being None. As noted in the following links, it fails:
https://stackoverflow.com/questions/16244923/how-to-make-a-custom-exception-class-with-multiple-init-args-pickleable

The error trace is: 
Exception in thread Thread-3:
Traceback (most recent call last):
  File "/usr/lib64/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/usr/lib64/python3.6/threading.py", line 864, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/lib64/python3.6/multiprocessing/pool.py", line 463, in _handle_results
    task = get()
  File "/usr/lib64/python3.6/multiprocessing/connection.py", line 252, in recv
    return _ForkingPickler.loads(r)
TypeError: __init__() missing 1 required positional argument: 'expected'


Fix proposed: 
Make the "expected" parameter of the IncompleteReadError exception class optional.
msg306270 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-15 14:45
This isn't so simple.

>>> from asyncio.streams import IncompleteReadError
>>> import pickle
>>> e1 = IncompleteReadError(b'abc', 10)
>>> e2 = pickle.loads(pickle.dumps(e1))
>>> print(e1)
3 bytes read on a total of 10 expected bytes
>>> print(e2)
44 bytes read on a total of None expected bytes
msg306289 - (view) Author: (badouxn) * Date: 2017-11-15 17:32
My first fix was indeed wrong. 
Upon looking further into it, it seems to me that the problem come from the fact that the call to super is not done with the right argument. 
Upon unpickling, the argument that will be passed to the __init__ is the string representation built on line 33-34.
That's why, when leaving expected as optional the number returned is 44, the length of the string. 

I went looking for similar Exception class in the code base and found the MaybeEncodingError in multiprocessing/pool.py 

By replacing the Error content with this one I don't have any error anymore. The message is kept identical. 
'''
     def __init__(self, partial, expected):
         super().__init__(partial, expected)
         self.partial = partial
         self.expected = expected
     def __str__(self):
         return ("%d bytes read on a total of %r expected bytes" % (len(partial), expected))
'''

Does such a fix looks correct to you ?
msg306292 - (view) Author: (badouxn) * Date: 2017-11-15 18:14
Typo in the last comment. The code should be:

'''
     def __init__(self, partial, expected):
         super().__init__(partial, expected)
         self.partial = partial
         self.expected = expected
     def __str__(self):
         return ("%d bytes read on a total of %r expected bytes" % (len(self.partial), self.expected))
'''
msg306297 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-15 18:48
> Does such a fix looks correct to you ?

No, this fix won't restore exception.args properly.  I made a PR with a fix.
msg306305 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-11-15 19:52
There is the same issue with LimitOverrunError.

I'm not sure that we should keep args the same. It affects __str__ and __repr__. badouxn overrides __str__. And the result of __repr__ currently looks like the exception constructor, but actually is not compatible with it.

>>> IncompleteReadError(b'abc', 10)
IncompleteReadError('3 bytes read on a total of 10 expected bytes',)
msg306316 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-15 22:14
New changeset 43605e6bfa8d49612df4a38460d063d6ba781906 by Yury Selivanov in branch 'master':
bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable #4409
https://github.com/python/cpython/commit/43605e6bfa8d49612df4a38460d063d6ba781906
msg306324 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-11-16 00:28
New changeset f35076a002b958f991d180d6f945344cc5ab3900 by Yury Selivanov (Miss Islington (bot)) in branch '3.6':
bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable GH-4409 (#4411)
https://github.com/python/cpython/commit/f35076a002b958f991d180d6f945344cc5ab3900
History
Date User Action Args
2017-11-16 00:28:28yselivanovsetmessages: + msg306324
2017-11-15 22:15:15yselivanovsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-11-15 22:14:39python-devsetpull_requests: + pull_request4361
2017-11-15 22:14:30yselivanovsetmessages: + msg306316
2017-11-15 19:52:59serhiy.storchakasetmessages: + msg306305
2017-11-15 18:48:02yselivanovsetmessages: + msg306297
2017-11-15 18:46:04yselivanovsetpull_requests: + pull_request4358
2017-11-15 18:14:21badouxnsetmessages: + msg306292
2017-11-15 17:32:49badouxnsetmessages: + msg306289
2017-11-15 14:45:45serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg306270
2017-11-15 13:12:31badouxnsetkeywords: + patch
stage: patch review
pull_requests: + pull_request4353
2017-11-15 13:10:54badouxnsetversions: + Python 3.7
2017-11-15 13:10:20badouxnsettitle: Impossible to unpickle IncompleteReadError -> Error when unpickling IncompleteReadError
2017-11-15 12:53:23badouxncreate