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 TrueTime class to improve synchronization logic #51

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 85 additions & 12 deletions packages/true-time/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,108 @@

import { TrueTime } from '.';

vi.useFakeTimers();
const DAYS_30 = 2_592_000_000;
const MINUTES_18 = 1_080_000;

describe('TrueTime', () => {
describe('tidal true time', () => {
const trueTime = new TrueTime('https://api.tidal.com/v1/ping');

beforeAll(async () => {
await trueTime.synchronize();
afterEach(() => {
vi.restoreAllMocks();
vi.clearAllTimers();
});

describe('synchronize', () => {
it('re-syncs time with server if it has not ben done for 1M ms', async () => {
const _trueTime = new TrueTime('https://api.tidal.com/v1/ping');

await _trueTime.synchronize();

vi.useFakeTimers();
vi.setSystemTime(new Date(Date.now() + MINUTES_18));

const spy = vi.spyOn(_trueTime, 'setServerTime');

await _trueTime.synchronize();

expect(spy).toBeCalled();
});
});

describe('driftDiff', () => {
it('returns the drift diff', () => {
const _trueTime = new TrueTime('https://api.tidal.com/v1/ping');
const diff = _trueTime.driftDiff();

expect(diff).toBeLessThan(1);
});

it('returns the current drift when performance.now() is out of sync', () => {
const oldTrueTime = new TrueTime('https://api.tidal.com/v1/ping');

vi.spyOn(oldTrueTime, 'timeOrigin').mockReturnValue(
Date.now() - DAYS_30 - performance.now(),
);

// eslint-disable-next-line vitest/valid-expect
expect(oldTrueTime.driftDiff()).to.be.closeTo(DAYS_30, 1000);
});
});

describe('currentDrift', () => {
it('returns the current drift when Date.now() is out of sync', () => {
vi.useFakeTimers();
vi.setSystemTime(new Date(Date.now() - DAYS_30));

// eslint-disable-next-line vitest/valid-expect
expect(trueTime.currentDrift()).to.be.closeTo(DAYS_30, 1000);
});

it('returns the current drift when performance.now() is out of sync', () => {
const oldTrueTime = new TrueTime('https://api.tidal.com/v1/ping');

vi.spyOn(oldTrueTime, 'timeOrigin').mockReturnValue(
Date.now() - DAYS_30 - performance.now(),
);

// eslint-disable-next-line vitest/valid-expect
expect(oldTrueTime.currentDrift()).to.be.closeTo(DAYS_30, 1000);
});
});

describe('now', () => {
it('returns the Date.now() value adjusted to server time', () => {
const thirtyDays = 2_592_000_000;
it('returns a valid timestamp when client clock is many days behind', async () => {
vi.useFakeTimers();
vi.setSystemTime(new Date(Date.now() - DAYS_30));

vi.setSystemTime(new Date(Date.now() - thirtyDays));
await trueTime.synchronize();

// Assert that TrueTime has adjusted
expect(trueTime.now()).not.toEqual(Date.now());
// eslint-disable-next-line vitest/valid-expect
expect(trueTime.now()).to.be.closeTo(Date.now(), 1000);
});

const diff = Math.abs(trueTime.now() - Date.now());
it('returns a valid timestamp when performance timeOrigin is many days behind', async () => {
vi.clearAllTimers();

const oldTrueTime = new TrueTime('https://api.tidal.com/v1/ping');

vi.spyOn(oldTrueTime, 'timeOrigin').mockReturnValue(
Date.now() - DAYS_30 - performance.now(),
);

await oldTrueTime.setServerTime();

// Check the diff and allow for 100 ms offset due to test timings.
// Chai style assertion that works, but is unexpected:
// eslint-disable-next-line vitest/valid-expect
expect(diff).to.be.closeTo(thirtyDays, 1000);
expect(oldTrueTime.now()).to.be.closeTo(Date.now(), 1000);
});
});

describe('timestamp', () => {
beforeAll(async () => {
await trueTime.synchronize();
});

test('returns adjusted time at mark', () => {
performance.mark('before time travel');

Expand Down
51 changes: 40 additions & 11 deletions packages/true-time/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,28 @@
/* eslint-disable no-restricted-syntax */
export class TrueTime {
#clientStartTime: number;
#initialDrift: number;

#serverTime?: number;

#synced = false;
#synced: number | undefined;

#url: URL;

constructor(url: string) {
this.#url = new URL(url);
this.#clientStartTime = performance.now();
this.#initialDrift = Math.abs(
Date.now() - (this.timeOrigin() + this.#clientStartTime),
);
}

currentDrift() {
return Math.abs(Date.now() - (this.timeOrigin() + performance.now()));
}

driftDiff() {
return Math.abs(this.#initialDrift - this.currentDrift());
}

now(highResTimeStamp: DOMHighResTimeStamp = performance.now()): number {
Expand All @@ -27,26 +40,42 @@ export class TrueTime {
);
}

async setServerTime() {
try {
const response = await fetch(this.#url);

if (response.ok && response.headers.has('date')) {
this.#serverTime = new Date(response.headers.get('date')!).getTime();
// eslint-disable-next-line no-restricted-syntax
this.#synced = Date.now();
}
} catch (error) {
console.error(error);
}
}

/**
* Use this method to synchronize time with the server.
*
* @param url - server url
*/
async synchronize() {
if (this.#synced) {
// Synchronize at max once every 1 000 000 miliseconds
Copy link
Contributor

Choose a reason for hiding this comment

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

If this should be allowed to run more than once, I can't see how it can work if not also setting clientStartTime?

// eslint-disable-next-line no-restricted-syntax
if (
(this.#synced && Math.abs(this.#synced - Date.now()) < 1_000_000) ||
(this.#synced &&
Math.abs(this.#initialDrift - this.currentDrift()) < 1_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

This "feels" like it is getting complicated, are we sure there are any benefits to not just replacing performance.now() with Date.now() and keeping this module simple?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we'd have to re-implement performance.mark() with Date.now too... :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it is a bit unfortunate that that is mixed in here, makes this more complex than it should be :-(

Might need to think a bit how we can avoid that..

Copy link
Contributor

Choose a reason for hiding this comment

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

I solved it in the other PR by setting startTime in each performance.mark() call, we can also consider making a trueTime.mark() that abstracts that maybe?

) {
return;
}

try {
const response = await fetch(this.#url);
return this.setServerTime();
}

if (response.ok && response.headers.has('date')) {
this.#serverTime = new Date(response.headers.get('date')!).getTime();
this.#synced = true;
}
} catch (error) {
console.error(error);
}
// Just exported to be able to mock from test.
timeOrigin() {
return performance.timeOrigin;
}

timestamp(markName: string, detail?: string): number | undefined {
Expand Down
Loading