Skip to content

use SystemTime instead of Instant #2406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

laptou
Copy link
Contributor

@laptou laptou commented Dec 28, 2021

This branch is based on the master branch in my fork, so it also contains the changes from #2405.
In this PR, I changed all instances of Instant to SystemTime in the Kademlia behaviour, because I am trying to make a RecordStore that serializes the records to an SQLite database, but Instants are extremely resistant to serialization. For now, I have only modified the structures used by Kademlia and their transitive dependencies, so there are still many uses of Instant in the ping, mDNS, and gossipsub behaviours.

Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only use Instant across the codebase because SystemTime is a very recent addition.

I can't think of any downside on using SystemTime over Instant so feel free to go ahead with these changes :)

@@ -50,6 +49,7 @@ use libp2p_swarm::{
use log::{debug, info, warn};
use smallvec::SmallVec;
use std::collections::{BTreeMap, HashSet, VecDeque};
use std::time::SystemTime;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IF we end up merging this, we need to use instant::SystemTime here.

@@ -697,7 +697,7 @@ where
let inner = QueryInner::new(info);
let id = self.queries.add_iter_closest(target.clone(), peers, inner); // (*)

// Instantly finish the query if we already have enough records.
// SystemTimely finish the query if we already have enough records.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an erroneous change :D

@@ -721,7 +721,7 @@ where
/// After the initial publication of the record, it is subject to (re-)replication
/// and (re-)publication as per the configured intervals. Periodic (re-)publication
/// does not update the record's expiration in local storage, thus a given record
/// with an explicit expiration will always expire at that instant and until then
/// with an explicit expiration will always expire at that SystemTime and until then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too.

@mxinden
Copy link
Member

mxinden commented Dec 29, 2021

RecordStore that serializes the records to an SQLite database

Great to see this happening @laptou!

I think we only use Instant across the codebase because SystemTime is a very recent addition.

SystemTime is not guaranteed to be monotonic:

Distinct from the Instant type, this time measurement is not monotonic. This means that you can save a file to the file system, then save another file to the file system, and the second file has a SystemTime measurement earlier than the first. In other words, an operation that happens after another operation in real time may have an earlier SystemTime!

https://doc.rust-lang.org/nightly/std/time/struct.SystemTime.html

I am not sure whether we rely on this property anywhere within the codebase.

I am trying to make a RecordStore that serializes the records to an SQLite database

Would it not be enough to use SystemTime in the serialization logic only, i.e. transform an Instant to a SystemTime via Duration when serializing and deserializing a record?

@tomaka
Copy link
Member

tomaka commented Dec 29, 2021

Replacing Instant with SystemTime is most certainly wrong. At any point, the value returned by SystemTime::now() can change. Maybe one moment SystemTime::now() returns December 29th 2021, and the next moment it returns June 2nd 1995, you can't know.

In order to serialize an Instant, one proper way is to do something like:

let now = Instant::now();
let now_system_time = SystemTime::now();
if instant > now {
    now_system_time + (instant - now)
} else {
    now_system_time - (now - instant)
}

@thomaseizinger
Copy link
Contributor

Learned something today, thanks for the comments!

@laptou laptou closed this Jan 11, 2022
@laptou laptou deleted the kad-use-systemtime branch January 11, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants