Skip to content
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

refactor: Replace Instant with SystemTime for Records and related code #5856

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rishiad
Copy link

@rishiad rishiad commented Feb 10, 2025

Description

This PR refactors the record storage code to use web_time::SystemTime instead of Instant in record types (e.g. in ProviderRecord and Record). The primary motivation is that Instant isn’t serializable—making it unsuitable for persistent storage—whereas SystemTime can be serialized by converting it to seconds since the Unix epoch. This approach was suggested in #4817

While the non-monotonic nature of SystemTime (i.e. it can fail due to system clock adjustments) is a known trade-off, this is less problematic over short durations.


Notes & Open Questions

  • One drawback of switching to SystemTime is that its API returns a Result when performing operations like duration calculations. In practice, this means there are edge cases where computing a duration might yield an error (for example, if the system clock has been adjusted unexpectedly). In contrast, Chrono’s DateTime subtraction always produces a signed TimeDelta (positive or negative) without error. I am open to rewriting this PR using Chrono if the reviewers agree with the rationale.
  • Please review the handling of potential errors from SystemTime operations.
  • If there are any concerns regarding the potential impact of non-monotonic behavior on duration comparisons, I’d like to discuss possible mitigation strategies.

Change Checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • A changelog entry has been made in the appropriate crates.

@@ -43,7 +43,6 @@ use libp2p_swarm::{
};
use thiserror::Error;
use tracing::Level;
use web_time::Instant;
Copy link
Member

Choose a reason for hiding this comment

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

To my knowledge, SystemTime is not available on wasm targets (it would panic when being used at runtime). In this case, we should use web_time::SystemTime in its place, no?

Copy link
Author

Choose a reason for hiding this comment

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

yeh that sounds like a better idea.

@dariusc93
Copy link
Member

Thanks for the PR. Left a small comment.

@rishiad rishiad requested a review from dariusc93 February 10, 2025 06:12
@rishiad
Copy link
Author

rishiad commented Feb 10, 2025

sorry, forgot to add the Cargo lock file in the last commit.

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Hi Rishi, thanks for this!
it seems web_stime::SystemTime also doesn't impl Serialize.
Reading serde-rs/serde#1375 and not being familiar with the need use-case, do we need to Serialize Record::expires and ProviderRecord::expires if not, we could use skip_serialize. If we do, I think I'd rather go serde-rs/serde#1375 (comment).

@rishiad
Copy link
Author

rishiad commented Feb 10, 2025

Hi Rishi, thanks for this! it seems web_stime::SystemTime also doesn't impl Serialize. Reading serde-rs/serde#1375 and not being familiar with the need use-case, do we need to Serialize Record::expires and ProviderRecord::expires if not, we could use skip_serialize. If we do, I think I'd rather go serde-rs/serde#1375 (comment).

Hmm, if a record is serialized and then kicked off memory then its expiry timestamp would be lost and when its bought back after deserialization, the expiry timestamp would be set to the default value, which breaks the intended behvaious that rely on the expiry field of the records (eg. garbage collecting expired records)

@rishiad
Copy link
Author

rishiad commented Feb 10, 2025

However, I do think Chrono's UTC timestamp would give us wasam friendly timestamp which is also serializable

@dariusc93
Copy link
Member

Hi Rishi, thanks for this! it seems web_stime::SystemTime also doesn't impl Serialize. Reading serde-rs/serde#1375 and not being familiar with the need use-case, do we need to Serialize Record::expires and ProviderRecord::expires if not, we could use skip_serialize. If we do, I think I'd rather go serde-rs/serde#1375 (comment).

I would be wrong but it does look like it impl Serialize in https://github.com/daxpedda/web-time/blob/main/src/time/serde.rs

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.

3 participants