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

Replace chrono with near-create inside near-network #5175

Closed
wants to merge 20 commits into from
Closed

Replace chrono with near-create inside near-network #5175

wants to merge 20 commits into from

Conversation

pmnoxx
Copy link
Contributor

@pmnoxx pmnoxx commented Nov 8, 2021

As discussed with @matklad, we discussed implementing our of crate for time.

Currently we use 4 methods to represent time chrono (Utc::now()) (Datetime<Utc>) , SystemTime, Instant::now, u64, they all have their issues.

  • We don't want to use external chrono library, it contains timezone information, we only care about Utc time
  • Instant is written in such a way, that it's not possible to convert it to unix timestamp. However, it's monotonic, which is good.
  • SystemTime works, but it's not monitonic. It's possible that if we measure time at time a then at time b; the duration b-a will be negative, causing our code to fail.
  • We want to have a way to mock time. We can't use global instance, because tests run in parallel.
  • We want to have a single source of XXX::now(), which we can mock. Ideally, we would be able to ensure that ::now() is correct, by grepping code and checking that we see NearClock::now() in all places that time is used. This will make it easy to check that we use correct time anywhere.
  • We want to use a saturating Duration, so any duration such that a < b, created by a - b, will default to 0, if the difference were to become nagative. This will reduce chances of any crashed happening, when negative time is not expected.

We are going to create our own wrapper around SystemTime and Duration. This will allow us to do the mocking properly.

Note:
If we decide to ship it, I'll split this PR into two parts:

See #5174

@pmnoxx pmnoxx self-assigned this Nov 8, 2021
@pmnoxx pmnoxx linked an issue Nov 8, 2021 that may be closed by this pull request
4 tasks
@pmnoxx pmnoxx requested a review from mzhangmzz as a code owner November 8, 2021 19:24
@pmnoxx pmnoxx requested a review from frol as a code owner November 9, 2021 08:01
@pmnoxx pmnoxx changed the title Create near-clock crate, which we can use for mocking time Replace chrono with near-create inside near-network Nov 9, 2021
@pmnoxx pmnoxx mentioned this pull request Nov 9, 2021
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Hm, given that what we want to do here is a "proper" mocked clock, I think it makes sense to try to avoid thread-locals, and use proper dependency injection. That is, to have two types: a clock (which returns time) and a time (which doesn't have a global now associated function):

#[derive(Clone)]
pub struct Clock {
    repr: ClockRepr,
}

#[derive(Clone)]
enum ClockRepr {
    Real,
    Mocked { now: Arc<AtomicU64> },
}

#[derive(Clone, Copy)]
pub struct Time {
    inner: SystemTime,
}

impl Clock {
    pub fn real_clock() -> Clock {
        Clock { repr: ClockRepr::Real }
    }

    pub fn mocked_clock() -> (Clock, Arc<AtomicU64>) {
        let now = Arc::new(AtomicU64::new(0));
        let clock = Clock { repr: ClockRepr::Mocked { now: Arc::clone(&now) } };
        (clock, now)
    }

    pub fn now(&self) -> Time {
        let inner = match &self.repr {
            ClockRepr::Real => SystemTime::now(),
            ClockRepr::Mocked { now } => {
                std::time::UNIX_EPOCH + Duration::from_nanos(now.load(Ordering::Relaxed))
            }
        };
        Time { inner }
    }
}

impl Time {
    #[deprecated(
        reason = "deliberately not implementing, use `now` method on the appropriate `Clock` instance."
    )]
    pub fn now() -> Time {
        unimplemented!(
            "deliberately not implementing, use `now` method on the appropriate `Clock` instance."
        )
    }

    pub fn duration_since(&self, start: Time) -> Duration {
        self.inner.duration_since(self.inner).unwrap_or_default()
    }
}

I think this might be a way to slice the work between this PR and #5123. #5123 is the simplest, stupidest thing we can do to unblock fuzzing of whatever specific feature we want to fuzz, and this PR is the proper, long-term solution which would serve us until at least April 2262.

utils/near-clock/src/lib.rs Outdated Show resolved Hide resolved
utils/near-clock/src/lib.rs Outdated Show resolved Hide resolved
@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 9, 2021

Hm, given that what we want to do here is a "proper" mocked clock, I think it makes sense to try to avoid thread-locals, and use proper dependency injection. That is, to have two types: a clock (which returns time) and a time (which doesn't have a global now associated function):

It depends on how we want to use time function. I see 2 options:

  • Use one instance of time, which can be mocked or not, and propagate it everywhere throughout the code
  • Use thread_local to exchange information between threads. On the main thread we will have a pointer to a helper data structure used for mocking time. Whenever we start a new thread, we would update that pointer, to point to the same data structure as used by the main thread (for example when starting new Actor)

@pmnoxx
Copy link
Contributor Author

pmnoxx commented Nov 9, 2021

@matklad I see. I talked with @yordanmadzhunkov I'll close this PR for now. We need to first finish how we mock time, we will have to do this some other way later.

@pmnoxx pmnoxx closed this Nov 9, 2021
@pmnoxx pmnoxx deleted the piotr-add-near-time branch November 13, 2021 06:24
@pmnoxx pmnoxx restored the piotr-add-near-time branch November 13, 2021 20:55
@pmnoxx pmnoxx reopened this Nov 13, 2021
pmnoxx and others added 2 commits November 13, 2021 13:00
@pmnoxx pmnoxx closed this Nov 14, 2021
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.

Implement near-time - a near create for time
2 participants