Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

datetime returned by Deserializer.deserialize_rfc() is not picklable #205

Closed ivanst0 closed 4 years ago

ivanst0 commented 4 years ago

Repro

import pickle
from msrest.serialization import Deserializer

datetime_rfc = "Mon, 25 May 2020 11:00:00 GMT"
datetime1 = Deserializer.deserialize_rfc(datetime_rfc)
print("datetime1: %s" % datetime1)
pickled = pickle.dumps(datetime1)
datetime2 = pickle.loads(pickled)
print("datetime2: %s" % datetime2)

Output (msrest 0.6.13)

datetime1: 2020-05-25 11:00:00+00:00
datetime2: 2020-05-25 11:00:00+00:00

Output (msrest 0.6.14)

datetime1: 2020-05-25 11:00:00+00:00
Traceback (most recent call last):
  File "d:\__temp\repro\main.py", line 8, in <module>
    datetime2 = pickle.loads(pickled)
TypeError: __init__() missing 1 required positional argument: 'offset'

Details

This regression was introduced in https://github.com/Azure/msrest-for-python/pull/201.

After that change, in the example above timedate1 is not picklable because timedate1.tzinfo contains an instance of _FixedOffset which is not picklable itself. pickle.dumps(datetime1) invokes timedate1.tzinfo.__reduce__(). _FixedOffset class doesn't define the __reduce__() method and the implementation from its parent class is used. tzinfo.__reduce__() assumes that the class implements __getinitargs__() method. This is true for datetime.timezone, but not for _FixedOffset. Eventually, pickle.loads(pickled) tries to call _FixedOffset.__init__() without the required offset argument, resulting in a TypeError.

In practice the issue happens when trying to pickle/unpickle any object containing a datetime generated by Deserializer.deserialize_rfc(), e.g. with multiprocessing.

Potential solutions

  1. Implement _FixedOffset.__getinitargs__().
  2. Implement _FixedOffset.__reduce__().
  3. Make _FixedOffset use the default implementation of __reduce__(), instead of one inherited from datetime.tzinfo: __reduce__ = object.__reduce__

Once Python 2.7 compatibility is no longer required, datetime.timezone can be used instead of _FixedOffset.

lmazuel commented 4 years ago

Thanks for the report! I made a PR for the fix. You also made me realize that I should use Python 3 when I can, so I use timezone is I can and use _FixedOffset only on Py 2.7. Thanks again for the detailled report! You made the unittests actually :)

lmazuel commented 4 years ago

Will ship part of 0.6.15 this week.

thanks again for the report :)