gh-103333: Pickle the keyword attributes of AttributeError

[csm10495]

gh-103333: Pickle the keyword attributes of AttributeError

This should fix the case where pickling/unpickling an AttributeError loses the name/obj fields.

If this looks ok, I'd be happy to add a unit test, blurb, etc.

[Eclips4]

Hello!
There's would be nice to have a tests for it case.

[csm10495]

Sure, I'll write tests, update blurb once I know that this approach is likely to be accepted :)

[gaogaotiantian]

It won't be a rare case when the object is not picklable right? Won't that make a picklable Exception non-picklable with this change?

[csm10495]

In theory yeah, if the object isn't picklable it would not work well here. Though this format follows in line with how other exceptions are made to be picklable (like OSError). Is there a way to somehow check if the obj, etc. are picklable and set them in that case (and if not, just leave as None (as it is now))?

[csm10495]

I wonder if there should be some sort of pickle mode that pickles all it can and for the fields it couldn't pickle: set to None or something else. Maybe even it could recurse and make a new object with 'as many fields as it could serialize' again.

[gaogaotiantian]

I think OSError guarantees that all the arguments are picklable(at least when used correctly). I'm not 100% sure but I believe that some of the standard libraries rely on the fact that all the built-in Exceptions are picklable - multiprocesing is one of them if I remember correctly. Not sure if making a built-in Exception unpicklable is an option here. I think @iritkatriel has been working on Exceptions a lot and must have a much better understanding than I do.

[csm10495]

The original issue came from a user using AttributeError via multiprocessing and the .name attribute was lost via the pickle. .. maybe a middle ground is to just serialize .name and postpone the conversation on .obj?

[gaogaotiantian]

Having an Exception that loses attributes after pickling is definitely better than having one that is not picklable sometimes. Supporting pickling the name attribute seems reasonable to me, but the core devs are the final decision makers.

[csm10495]

Updated PR to not pickle obj. Also updated a test to hit this change as well.

[iritkatriel]

CC @serhiy-storchaka re pickle.

[csm10495]

@iritkatriel @serhiy-storchaka any chance for a merge or feedback?

[csm10495]

@iritkatriel @serhiy-storchaka please provide feedback and/merge. Thanks.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[gpshead]

I left some comments, but I don't feel confident judging what changes to something that gets pickled are valid compatibility wise as I generally avoid use of pickle outside of tightly controlled scenarios.

Will there be any compatibility issue with an AttributeError pickled by this PR being loaded in a Python process running an older Python version without this PR?

[csm10495]

@gpshead I updated based off your feedback. In terms of pickle-ability it works out well:

Python 3.12.0a7+ (heads/attributeerror_pickling-dirty:0ee8b8cb, May 10 2023, 19:09:08) [MSC v.1932 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> a = AttributeError(name='test123')
>>> print(a.name)
test123
>>> import pickle
>>> pickle.dumps(a)
b'\x80\x04\x95@\x00\x00\x00\x00\x00\x00\x00\x8c\x08builtins\x94\x8c\x0eAttributeError\x94\x93\x94)R\x94}\x94(\x8c\x04name\x94\x8c\x07test123\x94\x8c\x04args\x94)ub.'
>>> pickle.loads(_).name
'test123'
>>>

Then in Python 3.11 I try to use the dumped pickle bytes:

Python 3.11.0 (main, Oct 24 2022, 18:26:48) [MSC v.1933 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import pickle
>>> a = pickle.loads(b'\x80\x04\x95@\x00\x00\x00\x00\x00\x00\x00\x8c\x08builtins\x94\x8c\x0eAttributeError\x94\x93\x94)R
>>> a.name
'test123'
>>> pickle.dumps(AttributeError(name='hello'))
b'\x80\x04\x95"\x00\x00\x00\x00\x00\x00\x00\x8c\x08builtins\x94\x8c\x0eAttributeError\x94\x93\x94)R\x94.'

Then back in 3.12.0a7, using that pickle data works, albeit without the name field (which makes sense since the old Python doesn't pickle the field):

>>> pickle.loads(b'\x80\x04\x95"\x00\x00\x00\x00\x00\x00\x00\x8c\x08builtins\x94\x8c\x0eAttributeError\x94\x93\x94)R\x94.')
AttributeError()
>>> pickle.loads(b'\x80\x04\x95"\x00\x00\x00\x00\x00\x00\x00\x8c\x08builtins\x94\x8c\x0eAttributeError\x94\x93\x94)R\x94.').name

Does this seem ok for merge now?

[erlend-aasland]

PEP-7 nits

[csm10495]

Fixed nits. Thanks!

[iritkatriel]

There is a refleak now in this test, could it be this PR?

% ./python.exe -m test -R3:3 test_exceptions -m testAttributes 
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 3.46 Run tests sequentially
0:00:00 load avg: 3.46 [1/1] test_exceptions
beginning 6 repetitions
123456
......
test_exceptions leaked [18, 18, 18] references, sum=54
test_exceptions leaked [12, 12, 12] memory blocks, sum=36
test_exceptions failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_exceptions

Total duration: 256 ms
Tests result: FAILURE

[iritkatriel]

See #104454