golemfactory / concent

Repository for Concent Service sources
8 stars 7 forks source link

Use non-naive datetimes. #93

Closed rwrzesien closed 6 years ago

rwrzesien commented 6 years ago

Example from @jiivan:

>>> import time, calendar, datetime
>>> int(
    time.time()),
    calendar.timegm(time.gmtime()),
    int(datetime.datetime.utcnow().timestamp()),
    int(datetime.datetime.now().timestamp(),
)
(1518000142, 1518000142, 1517996542, 1518000142)

As you can see timestamps generated using utcnow() are not in UTC. But the problem is not in utcnow() but in the timestamp() method. Docs for datetime.timestamp() say:

Naive datetime instances are assumed to represent local time and this method relies on the platform C mktime() function to perform the conversion.

This means that every call to timestamp() on a naive datetime object gives a result in the wrong timezone. Our timestamps must be in UTC. Otherwise timestamps created by clients in different timezones won't match.

To fix this: 1) Create a helper function for getting a timestamp that represents the current time (an int value). 2) Find all places where we get current timestamp in our code and replace them with a call to the helper. 3) Create a helper function for converting a datetime object (should support both naive and timezone-aware) to a timestamp (an int value). 4) Find all calls to timestamp() in our code and replace them with a call to the helper.

rwrzesien commented 6 years ago

Updated description.

rwrzesien commented 6 years ago

@cameel What do you think about this one ? I see two options here:

  1. Create timezone aware datetimes and store them in database with timezone info.
  2. Create naive datetimes but in utc time, store them without timezone info, disable timezone support in django.

Do we even want/need to support timezones in the project ?

cameel commented 6 years ago

Nomally we don't support timezones in projects where all timestamps are generated by ourselves. We store all timestamps in UTC and convert them to user's local timezone in the UI depending on his preferences.

But it's different when we have to support timestamps submitted by users (which is always problematic if you need to rely on those timestamps because of all the typical problems with time synchronization). That's the case here. Golem messages have timestamps created by clients. So it really depends on Golem - if they allow timestamps with timezones, we must allow them too.

But I think it would be better not to support timezones in messages. This would simplify things. Please talk with @jiivan about this.

Other than messages, we don't have to support timezones and we coud disable tiem.

cameel commented 6 years ago

I've updated the description with the conclusions from our discussion with @jiivan and @shadeofblue on Rocket: