From 1b8c6db4db62759ae816c2671a5f3d2569c12859 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Wed, 14 Mar 2018 15:11:00 -0700 Subject: [PATCH 1/2] Fix for b/74749605: Cancel pending backoff operations when closing streams. --- packages/firestore/CHANGELOG.md | 6 ++++ packages/firestore/src/remote/backoff.ts | 12 +++++-- .../firestore/src/remote/persistent_stream.ts | 11 +++--- .../test/unit/specs/remote_store_spec.test.ts | 34 ++++++++++++++++++- .../firestore/test/unit/specs/spec_builder.ts | 12 +++++-- .../test/unit/specs/spec_test_runner.ts | 3 +- 6 files changed, 67 insertions(+), 11 deletions(-) diff --git a/packages/firestore/CHANGELOG.md b/packages/firestore/CHANGELOG.md index 4d602c741b2..13899391aa2 100644 --- a/packages/firestore/CHANGELOG.md +++ b/packages/firestore/CHANGELOG.md @@ -1,4 +1,10 @@ # Unreleased +- [fixed] Fixed a regression in the Firebase JS release 4.11.0 that could + cause a crash if a user signs out while the client is offline, resulting in + an error of "Attempted to schedule multiple operations with timer id + listen_stream_connection_backoff". + +# 0.3.5 - [changed] If the SDK's attempt to connect to the Cloud Firestore backend neither succeeds nor fails within 10 seconds, the SDK will consider itself "offline", causing get() calls to resolve with cached results, rather than diff --git a/packages/firestore/src/remote/backoff.ts b/packages/firestore/src/remote/backoff.ts index 38262358289..6a1ced7c794 100644 --- a/packages/firestore/src/remote/backoff.ts +++ b/packages/firestore/src/remote/backoff.ts @@ -87,9 +87,8 @@ export class ExponentialBackoff { * already, it will be canceled. */ backoffAndRun(op: () => Promise): void { - if (this.timerPromise !== null) { - this.timerPromise.cancel(); - } + // Cancel any pending backoff operation. + this.cancel(); // First schedule using the current base (which may be 0 and should be // honored as such). @@ -118,6 +117,13 @@ export class ExponentialBackoff { } } + cancel(): void { + if (this.timerPromise !== null) { + this.timerPromise.cancel(); + this.timerPromise = null; + } + } + /** Returns a random value in the range [-currentBaseMs/2, currentBaseMs/2] */ private jitterDelayMs(): number { return (Math.random() - 0.5) * this.currentBaseMs; diff --git a/packages/firestore/src/remote/persistent_stream.ts b/packages/firestore/src/remote/persistent_stream.ts index df573a763b0..35ee55bacc1 100644 --- a/packages/firestore/src/remote/persistent_stream.ts +++ b/packages/firestore/src/remote/persistent_stream.ts @@ -316,6 +316,9 @@ export abstract class PersistentStream< this.cancelIdleCheck(); + // Ensure we don't leave a pending backoff operation queued. + this.backoff.cancel(); + if (finalState !== PersistentStreamState.Error) { // If this is an intentional close ensure we don't delay our next connection attempt. this.backoff.reset(); @@ -461,10 +464,10 @@ export abstract class PersistentStream< this.state = PersistentStreamState.Backoff; this.backoff.backoffAndRun(async () => { - if (this.state === PersistentStreamState.Stopped) { - // Stream can be stopped while waiting for backoff to complete. - return; - } + assert( + this.state === PersistentStreamState.Backoff, + 'Backoff should have been canceled if we left the Backoff state.' + ); this.state = PersistentStreamState.Initial; this.start(listener); diff --git a/packages/firestore/test/unit/specs/remote_store_spec.test.ts b/packages/firestore/test/unit/specs/remote_store_spec.test.ts index 3b5183eb940..4cfbc0732e1 100644 --- a/packages/firestore/test/unit/specs/remote_store_spec.test.ts +++ b/packages/firestore/test/unit/specs/remote_store_spec.test.ts @@ -17,7 +17,7 @@ import { expect } from 'chai'; import { Query } from '../../../src/core/query'; import { Code } from '../../../src/util/error'; -import { doc, path } from '../../util/helpers'; +import { doc, path, resumeTokenForSnapshot } from '../../util/helpers'; import { describeSpec, specTest } from './describe_spec'; import { spec } from './spec_builder'; @@ -83,4 +83,36 @@ describeSpec('Remote store:', [], () => { .expectEvents(query, { added: [doc1] }) ); }); + + // TODO(b/72313632): This test is web-only because the Android / iOS spec + // tests exclude backoff entirely. + specTest( + 'Handles user changes while offline (b/74749605).', + ['no-android', 'no-ios'], + () => { + const query = Query.atPath(path('collection')); + const docA = doc('collection/a', 1000, { key: 'a' }); + return ( + spec() + .userListens(query) + + // close the stream (this should trigger retry with backoff; but don't + // run it in an attempt to reproduce b/74749605). + .watchStreamCloses(Code.UNAVAILABLE, { runBackoffTimer: false }) + + // Because we didn't let the backoff timer run and restart the watch + // stream, there will be no active targets. + .expectActiveTargets() + + // Change user (will shut down existing streams and start new ones). + .changeUser('abc') + // Our query should be sent to the new stream. + .expectActiveTargets({ query, resumeToken: '' }) + + // Close the (newly-created) stream as if it too failed (should trigger + // retry with backoff, potentially reproducing the crash in b/74749605). + .watchStreamCloses(Code.UNAVAILABLE) + ); + } + ); }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index b13afa61a33..b225c57283b 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -496,14 +496,22 @@ export class SpecBuilder { return this; } - watchStreamCloses(error: Code): SpecBuilder { + watchStreamCloses( + error: Code, + opts?: { runBackoffTimer: boolean } + ): SpecBuilder { + if (!opts) { + opts = { runBackoffTimer: true }; + } + this.nextStep(); this.currentStep = { watchStreamClose: { error: { code: mapRpcCodeFromCode(error), message: 'Simulated Backend Error' - } + }, + runBackoffTimer: opts.runBackoffTimer } }; return this; diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index d1985b7db14..a03f87f2ac0 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -700,7 +700,7 @@ abstract class TestRunner { ) ); // The watch stream should re-open if we have active listeners. - if (!this.queryListeners.isEmpty()) { + if (spec.runBackoffTimer && !this.queryListeners.isEmpty()) { await this.queue.runDelayedOperationsEarly( TimerId.ListenStreamConnectionBackoff ); @@ -1167,6 +1167,7 @@ export type SpecSnapshotVersion = TestSnapshotVersion; export type SpecWatchStreamClose = { error: SpecError; + runBackoffTimer: boolean; }; export type SpecWriteAck = { From 47379fdb1f9f5b4f9bd92d3ca3ac8a1b8d8aee75 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Wed, 14 Mar 2018 16:01:33 -0700 Subject: [PATCH 2/2] Respond to CR feedback. --- packages/firestore/src/remote/persistent_stream.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/firestore/src/remote/persistent_stream.ts b/packages/firestore/src/remote/persistent_stream.ts index 35ee55bacc1..0a8468d50e8 100644 --- a/packages/firestore/src/remote/persistent_stream.ts +++ b/packages/firestore/src/remote/persistent_stream.ts @@ -314,9 +314,11 @@ export abstract class PersistentStream< "Can't provide an error when not in an error state." ); + // The stream will be closed so we don't need our idle close timer anymore. this.cancelIdleCheck(); - // Ensure we don't leave a pending backoff operation queued. + // Ensure we don't leave a pending backoff operation queued (in case close() + // was called while we were waiting to reconnect). this.backoff.cancel(); if (finalState !== PersistentStreamState.Error) { @@ -464,6 +466,12 @@ export abstract class PersistentStream< this.state = PersistentStreamState.Backoff; this.backoff.backoffAndRun(async () => { + if (this.state === PersistentStreamState.Stopped) { + // We should have canceled the backoff timer when the stream was + // closed, but just in case we make this a no-op. + return; + } + assert( this.state === PersistentStreamState.Backoff, 'Backoff should have been canceled if we left the Backoff state.'