From cdc3307cb2043091e61e9cea5c0e86f9acfd0a61 Mon Sep 17 00:00:00 2001 From: Michael Peters Date: Wed, 19 Oct 2022 21:49:00 -0700 Subject: [PATCH] unverify during verify with dedup --- src/client/tests/webapp/auto-verifier.test.ts | 69 +++++++++++++++++++ src/client/webapp/auto-verifier.ts | 19 +++-- 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/src/client/tests/webapp/auto-verifier.test.ts b/src/client/tests/webapp/auto-verifier.test.ts index 4046339..9323256 100644 --- a/src/client/tests/webapp/auto-verifier.test.ts +++ b/src/client/tests/webapp/auto-verifier.test.ts @@ -1219,6 +1219,65 @@ describe('fetchAndVerifyIfNeeded tests', () => { expect(verifyFunc).toHaveBeenCalledTimes(1); }); + test('ab: a: primary with null, b: primary dedup, ab: unverify during verify, ab: resolve with trusted anyway', async () => { + const { + nextPrimary, nextTrusted, nextVerify, + primaryFunc, trustedFunc, verifyFunc + } = getManualsAndMocks(); + + const av = new AutoVerifier(primaryFunc, trustedFunc, verifyFunc); + + const a = fetchAndVerifyIfNeededManually(av, nextPrimary, nextTrusted, nextVerify); + + expect(av.primaryPromise).toBe(a.primary.promise); + expect(av.trustedPromise).toBe(a.trusted.promise); + expect(av.trustedStatus).toBe('fetching'); + expect(primaryFunc).toHaveBeenCalled(); + expect(trustedFunc).toHaveBeenCalled(); + + const b = fetchAndVerifyIfNeededManually(av, nextPrimary, nextTrusted, nextVerify); + + expect(av.primaryPromise).toBe(a.primary.promise); // doesn't change from a + expect(av.trustedPromise).toBe(a.trusted.promise); // doesn't change from a + expect(av.trustedStatus).toBe('fetching'); + + a.primary.resolve(null); + await disjoint(); + + expect(av.primaryPromise).toBe(null); + expect(av.trustedPromise).toBe(a.trusted.promise); + expect(av.trustedStatus).toBe('verifying'); + + expect(verifyFunc).toHaveBeenCalledTimes(0); + a.trusted.resolve(new BasicWE(2)); + await disjoint(); + + expect(verifyFunc).toHaveBeenCalledWith(null, new BasicWE(2)); + expect(av.primaryPromise).toBe(null); + expect(av.trustedPromise).toBe(a.trusted.promise); + expect(av.trustedStatus).toBe('verifying'); + + expect(a.result).toBeUndefined(); + expect(b.result).toBeUndefined(); + + av.unverify(); + a.verify.resolve(true); + await disjoint(); + + expect(a.result).toEqual(new BasicWE(2)); + expect(b.result).toEqual(new BasicWE(2)); + expect(av.primaryPromise).toBe(null); + expect(av.trustedPromise).toBe(null); + expect(av.trustedStatus).toBe('none'); + + await disjoint(); // sanity check + + // Even though there were two fetchAnd... calls, they were deduped such that each func was only called once + expect(primaryFunc).toHaveBeenCalledTimes(1); + expect(trustedFunc).toHaveBeenCalledTimes(1); + expect(verifyFunc).toHaveBeenCalledTimes(1); + }); + // TODO: Why not have fetcher and a simple dedup wrapper to remove some complexity from this function? // This would likely make it possible to get rid of the "else" block in tryResolveTrustedPromise // Make sure it would work properly for retry functionality @@ -1227,4 +1286,14 @@ describe('fetchAndVerifyIfNeeded tests', () => { // Make sure to do try/catch errors/rejections, verification failures, unverifies in the middle // Fetching after verified doesn't re-fetch trusted // Unverify during verify w/ dedups + // verifyFunc returns false + // verifyFunc returns false while different trusted succeeds? - this is not possible + // verifyFunc rejects + + // not possible (and therefore not tested) list + // verifyFunc returns false while awaiting a trusted promise + // - implies it's possible to enter the status === 'fetching' block + // while other verifyFunc is being called. verifyFunc is only + // called in the 'fetching' state + // - expected would be to return verify trusted result }); diff --git a/src/client/webapp/auto-verifier.ts b/src/client/webapp/auto-verifier.ts index 2a9133a..e18c905 100644 --- a/src/client/webapp/auto-verifier.ts +++ b/src/client/webapp/auto-verifier.ts @@ -277,7 +277,8 @@ export class AutoVerifier { // this actually could be quite common //console.warn('RARE ALERT: we got unverified during verification!'); // ** complexity: - // keep in mind that this will resolve 'unverified' promises with their resulting origTrustedPromise (which should be a good thing) + // if primaryUpToDate is false or we got unverified during verification, this path will still return + // the trustedResult that was passed to the verify function } if (!resolved) { @@ -318,17 +319,15 @@ export class AutoVerifier { return; } - const origVerifyPromise: Promise | null = this.verifyPromise; - await origVerifyPromise; // we don't care about the result, just that we wait for the verification to finish - if (this.verifyPromise !== origVerifyPromise) { - // we've been invalidated while we were waiting for the trusted result! + await this.verifyPromise; // we don't care about the result, just that we wait for the verification to finish + if (this.trustedPromise !== origTrustedPromise) { + // we've been invalidated while we were waiting for the verify! console.warn( 'ULTRA RARE ALERT: we got unverified while awaiting a verify promise another path was calling!', - origVerifyPromise, - this.verifyPromise, ); - await tryResolveTrustedPromise(); - return; + // ** complexity: + // if primaryUpToDate is false or we got unverified during verification, this path will still return + // the trustedResult that was passed to the verify function } if (!resolved) { @@ -356,7 +355,7 @@ export class AutoVerifier { resolved = true; } else { this.unverify() - console.warn('server request failed after returning cache value (or when already rejected)', e); + console.warn('rejection after returning cache value (or when already rejected)', e); } } },