zopefoundation / DateTime

This package provides a DateTime data type, as known from Zope. Unless you need to communicate with Zope APIs, you're probably better off using Python's built-in datetime module.
Other
19 stars 25 forks source link

tests fail in 2038 #56

Closed bmwiedemann closed 6 months ago

bmwiedemann commented 1 year ago

BUG/PROBLEM REPORT / FEATURE REQUEST

What I did:

osc co openSUSE:Factory/python-DateTime && cd $_
osc build --vm-type=kvm --noservice --clean --build-opt=--vm-custom-opt="-rtc base=2039-09-02T06:07:00" --alternative-project=home:bmwiedemann:reproducible openSUSE_Tumbleweed

What I expect to happen:

tests should keep working in the future (at least 16 years)

What actually happened:

similar to issue #41 in that there is a rounding error in DateTime-5.2 that can randomly break a test.

 =================================== FAILURES ===================================
 __________________________ DateTimeTests.test_pickle ___________________________

 self = <DateTime.tests.test_datetime.DateTimeTests testMethod=test_pickle>

     def test_pickle(self):
         dt = DateTime()
         data = pickle.dumps(dt, 1)
         new = pickle.loads(data)
         for key in DateTime.__slots__:
 >           self.assertEqual(getattr(dt, key), getattr(new, key))
 E           AssertionError: 2198556426235027 != 2198556426235026

 src/DateTime/tests/test_datetime.py:253: AssertionError
 =============================== warnings summary ===============================

What version of Python and Zope/Addons I am using:

openSUSE-Tumbleweed-20230729 python3.10

bmwiedemann commented 6 months ago

still there in DateTime-5.4

icemac commented 6 months ago

@d-maurer Do have an idea what's going wrong here?

d-maurer commented 6 months ago

Michael Howitz wrote at 2024-3-19 00:24 -0700:

@d-maurer Do have an idea what's going wrong here?

DateTime uses a float in its state representation (representing seconds since the epoch). A float has limited precision. This limited precision is the cause of the error as demontrated by the following transscript:

>>> from DateTime import DateTime
>>> f=DateTime()
>>> f.__setstate__((2198556426.235027, False, 'GMT+1'))
>>> f
DateTime('2039/09/02 07:07:6.235027 GMT+1')
>>> f.__getstate__()
(2198556426.235026, False, 'GMT+1')

As we see, the __getstate__ result has lost the last bit.

If we want to get rid of errors like this, we must avoid a float in the __getstate__ representation.

One possibility would be to replace the float by a pair "(integral) seconds since epoch", "remaining microseconds". Another one to replace the float by "microseconds since the epoch". to use

perrinjerome commented 6 months ago

For reference:

https://github.com/zopefoundation/DateTime/blob/e892a9d360abba6e8abe1c4a7e48f52efa9c1e72/src/DateTime/DateTime.py#L448-L468

this was set as a float to have shorter pickles, this was apparently wrong for this reason. In our case ( cc @fdiary ), we considered that it is wrong because of the precision loss and we are using a monkey patch to export the int in __getstate__ ( for this and other historical reasons ).

Unlike with protocol 1, a float is no longer serialized shorter pickle than an int with pickle protocol 3:

>>> pickletools.dis(pickle.dumps(2198556426.235026, 3))
    0: \x80 PROTO      3
    2: G    BINFLOAT   2198556426.235026
   11: .    STOP
highest protocol among opcodes = 2
>>> pickletools.dis(pickle.dumps(int(2198556426.235026 * 1000000.0), 3))
    0: \x80 PROTO      3
    2: \x8a LONG1      2198556426235026
   11: .    STOP
highest protocol among opcodes = 2

for the fix, I think we can consider exporting the int "as is" during __getstate__ and add more conditions during the __setstate__ to support the new format.

d-maurer commented 6 months ago

Jérome Perrin wrote at 2024-3-19 04:36 -0700:

... for the fix, I think we can consider exporting the int "as is" during __getstate__ and add more conditions during the __setstate__ to support the new format.

I assume this int represents "microseconds since epoch`. In this case, would you like to prepare a PR (as you apparently have already most of the code for it).

perrinjerome commented 6 months ago

I assume this int represents "microseconds since epoch`.

exactly:

https://github.com/zopefoundation/DateTime/blob/868de710aa9b88505c090a64e6676538ead452c2/src/DateTime/DateTime.py#L861-L865

I submitted https://github.com/zopefoundation/DateTime/pull/62 with this change we discussed.

I tried to run osc myself but I had errors when trying to create an account, @bmwiedemann can you try easily from the linked PR ?

bmwiedemann commented 6 months ago

looks good in a quick test.

icemac commented 6 months ago

Fix released in https://pypi.org/project/DateTime/5.5/