ionelmc / python-tblib

Serialization library for Exceptions and Tracebacks.
BSD 2-Clause "Simplified" License
164 stars 33 forks source link

Deserialize by using __new__ whenever possible #73

Open oldium opened 1 year ago

oldium commented 1 year ago

Using of the class constructor and passing it the args values is not always correct, the custom parameters could be different to the args actually recorded by the built-in Exception class. Try to use the __new__ to create class instance and initialize the args attribute afterwards to prevent calling constructor with wrong arguments. Other instance attributes are initialized in a standard way by pickle module, so this way the reconstruction of the exception is complete.

Subclasses of OSError are known exceptions to this, because they initialize read-only attributes from the constructor argument values, so usage of the new factory method leads to lost values of those read-only attributes.

Fixes: #65

oldium commented 1 year ago

Applied changes suggested by the check job.

oldium commented 1 year ago

Tests added

oldium commented 1 year ago

Looks like (first guess) that PyPy does not like test for built-in reducer. I will check it.

oldium commented 1 year ago

I created a bug report for PyPy https://github.com/pypy/pypy/issues/4026 regarding the builtin method check. For now, we could switch to inspect.isbuiltin() (it looks it is not only a BuiltinFunctionType check in PyPy) and skip the test on PyPy until it is fixed?

oldium commented 1 year ago

I did what I proposed - there are two more commits, one switching to inspect.isbuiltin() (i.e. no functional change) and second one disabling issue #64 test. This means that under PyPy the behaviour of tblib is unchanged, the bug is still there. I hope PyPy team fixes/addresses the issue soon.

oldium commented 1 year ago

In the PyPy bug report one suggestion was to use obj.__class__.__reduce__ is BaseException.__reduce__ and it works both in CPython and PyPy. See the latest commit, which fixes the problem of #64 also for PyPy. Now the tests should be all green 😁

oldium commented 1 year ago

I am done with this PR.

oldium commented 9 months ago

Squashed related commits, updated reasoning in description and updated naming in code (no functional change, just added one more test). Also added two fixes for the tox build job, feel free to cherry-pick it separately 😊

oldium commented 9 months ago

Rebased to latest master

oldium commented 4 months ago

Rebased to the latest master and as a bonus fixed build with Python 3.8, 3.9 and 3.10 on macos.

Enjoy 😁

stuaxo commented 4 months ago

@oldium not sure if you noticed, but PyPy fixed the isbultin bug you reported.

oldium commented 4 months ago

@stuaxo Are you sure it is fixed, or you found that the bug is closed?

stuaxo commented 3 months ago

@oldium ooh, good point, looking at the ticket again I am not sure - apologies for the noise.

oldium commented 3 months ago

Rebased on top of #79