Skip to content
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

Fix #6628 - JSONDecodeError are not deserializable #6629

Merged

Conversation

Tarty
Copy link
Contributor

@Tarty Tarty commented Jan 31, 2024

See issue #6628 for full bug-report


requests.exceptions.JSONDecodeError are not deserializable: calling pickle.dumps followed by pickle.loads will trigger an error.

This is particularly a problem in a process pool, as an attempt to decode json on an invalid json document will result in the entire process pool crashing.

This is due to the MRO of the requests.exceptions.JSONDecodeError class: the __reduce__ method called when pickling an instance is not the one from the JSON library parent: two out of three args expected for instantiation will be dropped, and the instance can't be deserialised.

By specifying in the class which parent __reduce__ method should be called, the bug is fixed as all args are carried over in the resulting pickled bytes.

requests.exceptions.JSONDecodeError are not deserializable: calling
`pickle.dumps` followed by `pickle.loads` will trigger an error.

This is particularly a problem in a process pool, as an attempt to
decode json on an invalid json document will result in the entire
process pool crashing.

This is due to the MRO of the `requests.exceptions.JSONDecodeError`
class: the `__reduce__` method called when pickling an instance is not
the one from the JSON library parent: two out of three args expected
for instantiation will be dropped, and the instance can't be
deserialised.

By specifying in the class which parent `__reduce__` method should be
called, the bug is fixed as all args are carried over in the resulting
pickled bytes.
Copy link
Member

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

This looks reasonable, thanks @Tarty!

@nateprewitt
Copy link
Member

We need to fix the CI but this should be set to go once that's resolved.

@nateprewitt nateprewitt added this to the 2.32.0 milestone Feb 21, 2024
@sigmavirus24
Copy link
Contributor

closing and re-opening to trigger a new build

@nateprewitt
Copy link
Member

Looks like tests are passing now, I'll go ahead and merge. Thanks again, @Tarty!

@nateprewitt nateprewitt merged commit 382fc2c into psf:main Feb 22, 2024
30 of 52 checks passed
@Tarty
Copy link
Contributor Author

Tarty commented Feb 29, 2024

Happy to contribute, thanks for merging the fix in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants