FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-32034: Make asyncio.IncompleteReadError pickleable. by 1st1 · Pull Request #4409 · python/cpython · GitHub

/ cpython Public

bpo-32034: Make asyncio.IncompleteReadError pickleable. - #4409

Merged
1st1 merged 3 commits into
python:masterfrom
1st1:32034
Nov 15, 2017
ConversationCommits (3)ChecksFiles changed
Merged

bpo-32034: Make asyncio.IncompleteReadError pickleable.#4409
1st1 merged 3 commits into
python:masterfrom
1st1:32034

Conversation

1st1 commented Nov 15, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

1st1 requested a review from asvetlov November 15, 2017 18:46
1st1 reopened this Nov 15, 2017

pitrou commented Nov 15, 2017

Copy link
Copy Markdown
Member

I think this would deserve backporting as well.

serhiy-storchaka reviewed Nov 15, 2017
View reviewed changes
Comment thread Lib/test/test_asyncio/test_streams.py Outdated

def test_IncompleteReadError_pickleable(self):
e = asyncio.IncompleteReadError(b'abc', 10)
e2 = pickle.loads(pickle.dumps(e))

serhiy-storchaka Nov 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Test with all supported protocols.

for proto in range(pickle.HIGHEST_PROTOCOL + 1):

1st1 Nov 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done

Comment thread Lib/asyncio/streams.py
self.expected = expected

def __reduce__(self):
return type(self), (self.partial, self.expected)

serhiy-storchaka Nov 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is other problem with this solution -- it loses all additional attributes.

1st1 Nov 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

asyncio doesn't set any additional attributes, so I think it's OK

serhiy-storchaka Nov 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What if somebody will create a subclass?

Adding self.__dict__ as the third component will save all attributes. But not slots.

bpo-26579 could help to support general cases. But pickling exceptions is a headache in any case.

1st1 Nov 15, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What if somebody will create a subclass?

Well, they then can handle pickling on their own. I mean in asyncio we control both exceptions and don't subclass them. If someone outside of asyncio decides to subclass or otherwise reuse these exceptions, it's entirely up to them.

I don't want to introduce any extra overhead here.

serhiy-storchaka Nov 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Then everything is fine. Except misleading repr, but this is another story.

1st1 reopened this Nov 15, 2017

1st1 commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

@Mariatta Not sure what's going on with this PR, but bedevere-bot keeps closing it whenever I push a commit...

1st1 reopened this Nov 15, 2017

pitrou commented Nov 15, 2017

Copy link
Copy Markdown
Member

I wonder why exceptions need special care at all to be picklable.

serhiy-storchaka commented Nov 15, 2017

Copy link
Copy Markdown
Member

I wonder why exceptions need special care at all to be picklable.

Due to BaseException.__reduce__ which uses args as arguments for recreating an exception (but loses keyword arguments and don't respect slots, __getstate__, __getnewargs__) on one side, and using the single argument for specifying a human-readable error message.

pitrou commented Nov 15, 2017

Copy link
Copy Markdown
Member

Can BaseException.__reduce__ be fixed?

serhiy-storchaka approved these changes Nov 15, 2017
View reviewed changes
Comment thread Lib/asyncio/streams.py
self.expected = expected

def __reduce__(self):
return type(self), (self.partial, self.expected)

serhiy-storchaka Nov 15, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Then everything is fine. Except misleading repr, but this is another story.

serhiy-storchaka commented Nov 15, 2017

Copy link
Copy Markdown
Member

Can BaseException.__reduce__ be fixed?

Maybe it can be improved (bpo-26579 can slightly help here). But this perhaps needs fixing BaseException constructors and other methods.

1st1 merged commit 43605e6 into python:master Nov 15, 2017

miss-islington commented Nov 15, 2017

Copy link
Copy Markdown
Contributor

Thanks @1st1 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 15, 2017
bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable py…
0780aad 
…thonGH-4409

(cherry picked from commit 43605e6)

1st1 commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

Thanks, Serhiy!

Mariatta commented Nov 15, 2017

Copy link
Copy Markdown
Member

@1S1 I'm so sorry! It's my fault.. The PR was closed because of this change to bedevere earlier today. 😞 I'll get that fixed ASAP

Mariatta mentioned this pull request Nov 15, 2017
Merged

1st1 commented Nov 15, 2017

Copy link
Copy Markdown
Member Author

@Mariatta it's not a problem at all! I just wanted you to know that there's some kind of bug here, that's all. Thanks for looking into this!

Mariatta commented Nov 16, 2017

Copy link
Copy Markdown
Member

Things don't seem to work well today :(
The backport PR for this is at #4411. Not sure why bedevere didn't pick that up. I'll remove the needs backport label.
Thanks :)

1st1 pushed a commit that referenced this pull request Nov 16, 2017
bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable GH…
f35076a 
…-4409 (#4411)

(cherry picked from commit 43605e6)
jimmylai added a commit to jimmylai/cpython that referenced this pull request Nov 17, 2017
Merge branch 'master' of https://github.com/python/cpython
d914928 
* 'master' of https://github.com/python/cpython: (550 commits)
  bpo-31867: Remove duplicates in default mimetypes. (python#4388)
  tokenizer: Remove unused tabs options (python#4422)
  bpo-31691: Specify where to find build instructions for the Windows installer (python#4426)
  Fix typo in atexit documentation. (pythonGH-4419)
  bpo-31702: Allow to specify rounds for SHA-2 hashing in crypt.mksalt(). (python#4110)
  bpo-32043: New "developer mode": "-X dev" option (python#4413)
  bpo-30349: Raise FutureWarning for nested sets and set operations (python#1553)
  bpo-32037: Use the INT opcode for 32-bit integers in protocol 0 pickles. (python#4407)
  bpo-30143: 2to3 now generates a code that uses abstract collection classes (python#1262)
  bpo-32030: Enhance Py_Main() (python#4412)
  bpo-32030: Split Py_Main() into subfunctions (python#4399)
  bpo-32034: Make IncompleteReadError & LimitOverrunError pickleable python#4409
  bpo-32025: Add time.thread_time() (python#4410)
  bpo-32018: Fix inspect.signature repr to follow PEP 8 (python#4408)
  bpo-30399: Get rid of trailing comma in the repr of BaseException. (python#1650)
  bpo-30950: Convert round() to Argument Clinic. (python#2740)
  bpo-32011: Revert "Issue python#15480: Remove the deprecated and unused TYPE_INT64 code from marshal." (python#4381)
  bpo-32023: Disallow genexprs without parenthesis in class definitions. (python#4400)
  bpo-31949: Fixed several issues in printing tracebacks (PyTraceBack_Print()). (python#4289)
  bpo-32032: Test both implementations of module-level pickle API. (python#4401)
  ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

serhiy-storchaka serhiy-storchaka approved these changes
asvetlov Awaiting requested review from asvetlov

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL