diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md index 34b56b97f21..b588fa2389d 100644 --- a/common/api-review/firestore.api.md +++ b/common/api-review/firestore.api.md @@ -211,6 +211,11 @@ export interface ExperimentalLongPollingOptions { timeoutSeconds?: number; } +// @public +export interface ExperimentalOptions { + sendWriteRequestsDelayMs?: number; +} + // @public export class FieldPath { constructor(...fieldNames: string[]); @@ -252,6 +257,7 @@ export type FirestoreLocalCache = MemoryLocalCache | PersistentLocalCache; // @public export interface FirestoreSettings { cacheSizeBytes?: number; + experimental?: ExperimentalOptions; experimentalAutoDetectLongPolling?: boolean; experimentalForceLongPolling?: boolean; experimentalLongPollingOptions?: ExperimentalLongPollingOptions; diff --git a/docs-devsite/_toc.yaml b/docs-devsite/_toc.yaml index 665222edb9d..383c2e92736 100644 --- a/docs-devsite/_toc.yaml +++ b/docs-devsite/_toc.yaml @@ -221,6 +221,8 @@ toc: path: /docs/reference/js/firestore_.documentsnapshot.md - title: ExperimentalLongPollingOptions path: /docs/reference/js/firestore_.experimentallongpollingoptions.md + - title: ExperimentalOptions + path: /docs/reference/js/firestore_.experimentaloptions.md - title: FieldPath path: /docs/reference/js/firestore_.fieldpath.md - title: FieldValue diff --git a/docs-devsite/firestore_.experimentaloptions.md b/docs-devsite/firestore_.experimentaloptions.md new file mode 100644 index 00000000000..51a1c846945 --- /dev/null +++ b/docs-devsite/firestore_.experimentaloptions.md @@ -0,0 +1,45 @@ +Project: /docs/reference/js/_project.yaml +Book: /docs/reference/_book.yaml +page_type: reference + +{% comment %} +DO NOT EDIT THIS FILE! +This is generated by the JS SDK team, and any local changes will be +overwritten. Changes should be made in the source code at +https://github.com/firebase/firebase-js-sdk +{% endcomment %} + +# ExperimentalOptions interface +Experimental options to configure the Firestore SDK. + +Note: This interface is "experimental" and is subject to change. + +See `FirestoreSettings.experimental`. + +Signature: + +```typescript +export declare interface ExperimentalOptions +``` + +## Properties + +| Property | Type | Description | +| --- | --- | --- | +| [sendWriteRequestsDelayMs](./firestore_.experimentaloptions.md#experimentaloptionssendwriterequestsdelayms) | number | The maximum amount of time, in milliseconds, to wait before sending a Firestore "write" request to the backend. If undefined then do not delay at all.A delay can be useful because it enables the in-memory "write pipeline" to gather together multiple write requests and send them in a single HTTP request to the backend, rather than one HTTP request per write request, as is done when by default, or when this property is undefined. Note that there is a hardcoded limit to the number of write requests that are sent at once, so setting a very large value for this property will not necessarily cause \_all\_ write requests to be sent in a single HTTP request; however, it \_could\_ greatly reduce the number of distinct HTTP requests that are used.The value must be an integer value strictly greater than zero and less than or equal to 10000 (10 seconds). A value of 200 is a good starting point to minimize write latency yet still enable some amount of batching.See https://github.com/firebase/firebase-js-sdk/issues/5971 for rationale and background information that motivated this option. | + +## ExperimentalOptions.sendWriteRequestsDelayMs + +The maximum amount of time, in milliseconds, to wait before sending a Firestore "write" request to the backend. If `undefined` then do not delay at all. + +A delay can be useful because it enables the in-memory "write pipeline" to gather together multiple write requests and send them in a single HTTP request to the backend, rather than one HTTP request per write request, as is done when by default, or when this property is `undefined`. Note that there is a hardcoded limit to the number of write requests that are sent at once, so setting a very large value for this property will not necessarily cause \_all\_ write requests to be sent in a single HTTP request; however, it \_could\_ greatly reduce the number of distinct HTTP requests that are used. + +The value must be an integer value strictly greater than zero and less than or equal to 10000 (10 seconds). A value of `200` is a good starting point to minimize write latency yet still enable some amount of batching. + +See https://github.com/firebase/firebase-js-sdk/issues/5971 for rationale and background information that motivated this option. + +Signature: + +```typescript +sendWriteRequestsDelayMs?: number; +``` diff --git a/docs-devsite/firestore_.firestoresettings.md b/docs-devsite/firestore_.firestoresettings.md index 6d24d583175..711e5b76bca 100644 --- a/docs-devsite/firestore_.firestoresettings.md +++ b/docs-devsite/firestore_.firestoresettings.md @@ -23,6 +23,7 @@ export declare interface FirestoreSettings | Property | Type | Description | | --- | --- | --- | | [cacheSizeBytes](./firestore_.firestoresettings.md#firestoresettingscachesizebytes) | number | NOTE: This field will be deprecated in a future major release. Use cache field instead to specify cache size, and other cache configurations.An approximate cache size threshold for the on-disk data. If the cache grows beyond this size, Firestore will start removing data that hasn't been recently used. The size is not a guarantee that the cache will stay below that size, only that if the cache exceeds the given size, cleanup will be attempted.The default value is 40 MB. The threshold must be set to at least 1 MB, and can be set to CACHE_SIZE_UNLIMITED to disable garbage collection. | +| [experimental](./firestore_.firestoresettings.md#firestoresettingsexperimental) | [ExperimentalOptions](./firestore_.experimentaloptions.md#experimentaloptions_interface) | Options that are "experimental", meaning that their semantics are subject to change at any time without notice, up to and including complete removal. | | [experimentalAutoDetectLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalautodetectlongpolling) | boolean | Configures the SDK's underlying transport (WebChannel) to automatically detect if long-polling should be used. This is very similar to experimentalForceLongPolling, but only uses long-polling if required.After having had a default value of false since its inception in 2019, the default value of this setting was changed in May 2023 to true in v9.22.0 of the Firebase JavaScript SDK. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to false, and please open a GitHub issue to share the problems that motivated you disabling long-polling auto-detection.This setting cannot be used in a Node.js environment. | | [experimentalForceLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalforcelongpolling) | boolean | Forces the SDK’s underlying network transport (WebChannel) to use long-polling. Each response from the backend will be closed immediately after the backend sends data (by default responses are kept open in case the backend has more data to send). This avoids incompatibility issues with certain proxies, antivirus software, etc. that incorrectly buffer traffic indefinitely. Use of this option will cause some performance degradation though.This setting cannot be used with experimentalAutoDetectLongPolling and may be removed in a future release. If you find yourself using it to work around a specific network reliability issue, please tell us about it in https://github.com/firebase/firebase-js-sdk/issues/1674.This setting cannot be used in a Node.js environment. | | [experimentalLongPollingOptions](./firestore_.firestoresettings.md#firestoresettingsexperimentallongpollingoptions) | [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options that configure the SDK’s underlying network transport (WebChannel) when long-polling is used.These options are only used if experimentalForceLongPolling is true or if experimentalAutoDetectLongPolling is true and the auto-detection determined that long-polling was needed. Otherwise, these options have no effect. | @@ -45,6 +46,16 @@ The default value is 40 MB. The threshold must be set to at least 1 MB, and can cacheSizeBytes?: number; ``` +## FirestoreSettings.experimental + +Options that are "experimental", meaning that their semantics are subject to change at any time without notice, up to and including complete removal. + +Signature: + +```typescript +experimental?: ExperimentalOptions; +``` + ## FirestoreSettings.experimentalAutoDetectLongPolling Configures the SDK's underlying transport (WebChannel) to automatically detect if long-polling should be used. This is very similar to `experimentalForceLongPolling`, but only uses long-polling if required. diff --git a/docs-devsite/firestore_.md b/docs-devsite/firestore_.md index 91d21e32708..2e87bc92027 100644 --- a/docs-devsite/firestore_.md +++ b/docs-devsite/firestore_.md @@ -168,6 +168,7 @@ https://github.com/firebase/firebase-js-sdk | [DocumentChange](./firestore_.documentchange.md#documentchange_interface) | A DocumentChange represents a change to the documents matching a query. It contains the document affected and the type of change that occurred. | | [DocumentData](./firestore_.documentdata.md#documentdata_interface) | Document data (for use with [setDoc()](./firestore_lite.md#setdoc_ee215ad)) consists of fields mapped to values. | | [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options that configure the SDK’s underlying network transport (WebChannel) when long-polling is used.Note: This interface is "experimental" and is subject to change.See FirestoreSettings.experimentalAutoDetectLongPolling, FirestoreSettings.experimentalForceLongPolling, and FirestoreSettings.experimentalLongPollingOptions. | +| [ExperimentalOptions](./firestore_.experimentaloptions.md#experimentaloptions_interface) | Experimental options to configure the Firestore SDK.Note: This interface is "experimental" and is subject to change.See FirestoreSettings.experimental. | | [FirestoreDataConverter](./firestore_.firestoredataconverter.md#firestoredataconverter_interface) | Converter used by withConverter() to transform user objects of type AppModelType into Firestore data of type DbModelType.Using the converter allows you to specify generic type arguments when storing and retrieving objects from Firestore.In this context, an "AppModel" is a class that is used in an application to package together related information and functionality. Such a class could, for example, have properties with complex, nested data types, properties used for memoization, properties of types not supported by Firestore (such as symbol and bigint), and helper functions that perform compound operations. Such classes are not suitable and/or possible to store into a Firestore database. Instead, instances of such classes need to be converted to "plain old JavaScript objects" (POJOs) with exclusively primitive properties, potentially nested inside other POJOs or arrays of POJOs. In this context, this type is referred to as the "DbModel" and would be an object suitable for persisting into Firestore. For convenience, applications can implement FirestoreDataConverter and register the converter with Firestore objects, such as DocumentReference or Query, to automatically convert AppModel to DbModel when storing into Firestore, and convert DbModel to AppModel when retrieving from Firestore. | | [FirestoreSettings](./firestore_.firestoresettings.md#firestoresettings_interface) | Specifies custom configurations for your Cloud Firestore instance. You must set these before invoking any other methods. | | [Index](./firestore_.index.md#index_interface) | (Public Preview) The SDK definition of a Firestore index. | diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index ea969c6b94c..1cdf6980eda 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -84,6 +84,7 @@ export { export { FirestoreSettings, PersistenceSettings } from './api/settings'; export type { PrivateSettings } from './lite-api/settings'; export { ExperimentalLongPollingOptions } from './api/long_polling_options'; +export { ExperimentalOptions } from './api/experimental_options'; export { DocumentChange, diff --git a/packages/firestore/src/api/experimental_options.ts b/packages/firestore/src/api/experimental_options.ts new file mode 100644 index 00000000000..a2862d742cd --- /dev/null +++ b/packages/firestore/src/api/experimental_options.ts @@ -0,0 +1,76 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Experimental options to configure the Firestore SDK. + * + * Note: This interface is "experimental" and is subject to change. + * + * See `FirestoreSettings.experimental`. + */ +export interface ExperimentalOptions { + /** + * The maximum amount of time, in milliseconds, to wait before sending a + * Firestore "write" request to the backend. If `undefined` then do not delay + * at all. + * + * A delay can be useful because it enables the in-memory "write pipeline" to + * gather together multiple write requests and send them in a single HTTP + * request to the backend, rather than one HTTP request per write request, as + * is done when by default, or when this property is `undefined`. Note that + * there is a hardcoded limit to the number of write requests that are sent at + * once, so setting a very large value for this property will not necessarily + * cause _all_ write requests to be sent in a single HTTP request; however, it + * _could_ greatly reduce the number of distinct HTTP requests that are used. + * + * The value must be an integer value strictly greater than zero and less than + * or equal to 10000 (10 seconds). A value of `200` is a good starting point + * to minimize write latency yet still enable some amount of batching. + * + * See https://github.com/firebase/firebase-js-sdk/issues/5971 for rationale + * and background information that motivated this option. + */ + sendWriteRequestsDelayMs?: number; +} + +/** + * Compares two `ExperimentalOptions` objects for equality. + */ +export function experimentalOptionsEqual( + options1: ExperimentalOptions, + options2: ExperimentalOptions +): boolean { + return ( + options1.sendWriteRequestsDelayMs === options2.sendWriteRequestsDelayMs + ); +} + +/** + * Creates and returns a new `ExperimentalOptions` with the same + * option values as the given instance. + */ +export function cloneExperimentalOptions( + options: ExperimentalOptions +): ExperimentalOptions { + const clone: ExperimentalOptions = {}; + + if (options.sendWriteRequestsDelayMs !== undefined) { + clone.sendWriteRequestsDelayMs = options.sendWriteRequestsDelayMs; + } + + return clone; +} diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index b47017bbc2e..5155be691bd 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -18,6 +18,7 @@ import { FirestoreSettings as LiteSettings } from '../lite-api/settings'; import { FirestoreLocalCache } from './cache_config'; +import { ExperimentalOptions } from './experimental_options'; import { ExperimentalLongPollingOptions } from './long_polling_options'; export { DEFAULT_HOST } from '../lite-api/settings'; @@ -114,4 +115,10 @@ export interface FirestoreSettings extends LiteSettings { * effect. */ experimentalLongPollingOptions?: ExperimentalLongPollingOptions; + + /** + * Options that are "experimental", meaning that their semantics are subject + * to change at any time without notice, up to and including complete removal. + */ + experimental?: ExperimentalOptions; } diff --git a/packages/firestore/src/core/component_provider.ts b/packages/firestore/src/core/component_provider.ts index 8a63509232c..9f59502285c 100644 --- a/packages/firestore/src/core/component_provider.ts +++ b/packages/firestore/src/core/component_provider.ts @@ -485,7 +485,8 @@ export class OnlineComponentProvider { onlineState, OnlineStateSource.RemoteStore ), - newConnectivityMonitor() + newConnectivityMonitor(), + cfg.databaseInfo.sendWriteRequestsDelayMs ); } diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index 0325f8166b6..c8fc6c82247 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -38,6 +38,8 @@ export class DatabaseInfo { * @param longPollingOptions Options that configure long-polling. * @param useFetchStreams Whether to use the Fetch API instead of * XMLHTTPRequest + * @param sendWriteRequestsDelayMs The delay, in milliseconds, to use before + * sending write requests over the wire in remote store. */ constructor( readonly databaseId: DatabaseId, @@ -48,7 +50,8 @@ export class DatabaseInfo { readonly forceLongPolling: boolean, readonly autoDetectLongPolling: boolean, readonly longPollingOptions: ExperimentalLongPollingOptions, - readonly useFetchStreams: boolean + readonly useFetchStreams: boolean, + readonly sendWriteRequestsDelayMs: number | null ) {} } diff --git a/packages/firestore/src/lite-api/components.ts b/packages/firestore/src/lite-api/components.ts index 436d2b5d4d8..23764f58857 100644 --- a/packages/firestore/src/lite-api/components.ts +++ b/packages/firestore/src/lite-api/components.ts @@ -119,6 +119,7 @@ export function makeDatabaseInfo( settings.experimentalForceLongPolling, settings.experimentalAutoDetectLongPolling, cloneLongPollingOptions(settings.experimentalLongPollingOptions), - settings.useFetchStreams + settings.useFetchStreams, + settings.experimental.sendWriteRequestsDelayMs ?? null ); } diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index a1bba373d13..a8b9d149a2f 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -19,6 +19,11 @@ import { EmulatorMockTokenOptions } from '@firebase/util'; import { FirestoreLocalCache } from '../api/cache_config'; import { CredentialsSettings } from '../api/credentials'; +import { + ExperimentalOptions, + cloneExperimentalOptions, + experimentalOptionsEqual +} from '../api/experimental_options'; import { ExperimentalLongPollingOptions, cloneLongPollingOptions, @@ -50,6 +55,10 @@ const MAX_LONG_POLLING_TIMEOUT_SECONDS = 30; // Whether long-polling auto-detected is enabled by default. const DEFAULT_AUTO_DETECT_LONG_POLLING = true; +// Set some maximum value for `sendWriteRequestsDelayMs` to avoid it being set +// to a value so large that it appears that write requests are never being sent. +const MAX_SEND_WRITE_REQUEST_DELAY_MS = 10000; + /** * Specifies custom configurations for your Cloud Firestore instance. * You must set these before invoking any other methods. @@ -83,6 +92,7 @@ export interface PrivateSettings extends FirestoreSettings { experimentalLongPollingOptions?: ExperimentalLongPollingOptions; useFetchStreams?: boolean; emulatorOptions?: { mockUserToken?: EmulatorMockTokenOptions | string }; + experimental?: ExperimentalOptions; localCache?: FirestoreLocalCache; } @@ -111,6 +121,7 @@ export class FirestoreSettingsImpl { readonly useFetchStreams: boolean; readonly localCache?: FirestoreLocalCache; + readonly experimental: ExperimentalOptions; // Can be a google-auth-library or gapi client. // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -178,6 +189,9 @@ export class FirestoreSettingsImpl { validateLongPollingOptions(this.experimentalLongPollingOptions); this.useFetchStreams = !!settings.useFetchStreams; + + this.experimental = cloneExperimentalOptions(settings.experimental ?? {}); + validateExperimentalOptions(this.experimental); } isEqual(other: FirestoreSettingsImpl): boolean { @@ -195,7 +209,8 @@ export class FirestoreSettingsImpl { other.experimentalLongPollingOptions ) && this.ignoreUndefinedProperties === other.ignoreUndefinedProperties && - this.useFetchStreams === other.useFetchStreams + this.useFetchStreams === other.useFetchStreams && + experimentalOptionsEqual(this.experimental, other.experimental) ); } } @@ -227,3 +242,26 @@ function validateLongPollingOptions( } } } + +function validateExperimentalOptions(options: ExperimentalOptions): void { + if (options.sendWriteRequestsDelayMs !== undefined) { + if (isNaN(options.sendWriteRequestsDelayMs)) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid sendWriteRequestsDelayMs: ` + + `${options.sendWriteRequestsDelayMs} (must not be NaN)` + ); + } + if ( + options.sendWriteRequestsDelayMs <= 0 || + options.sendWriteRequestsDelayMs > MAX_SEND_WRITE_REQUEST_DELAY_MS + ) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid sendWriteRequestsDelayMs: ` + + `${options.sendWriteRequestsDelayMs} (must be greater than zero ` + + `and less than or equal to ${MAX_SEND_WRITE_REQUEST_DELAY_MS})` + ); + } + } +} diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index 6f8aed0503e..2ed4043084c 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -28,7 +28,7 @@ import { TargetData } from '../local/target_data'; import { MutationResult } from '../model/mutation'; import { MutationBatch, MutationBatchResult } from '../model/mutation_batch'; import { debugAssert, debugCast } from '../util/assert'; -import { AsyncQueue } from '../util/async_queue'; +import { AsyncQueue, DelayedOperation, TimerId } from '../util/async_queue'; import { ByteString } from '../util/byte_string'; import { FirestoreError } from '../util/error'; import { logDebug } from '../util/log'; @@ -77,6 +77,11 @@ const enum OfflineCause { Shutdown } +export interface WritePipelineEntry { + mutationBatch: MutationBatch; + writeRequestSent: boolean; +} + /** * RemoteStore - An interface to remotely stored data, basically providing a * wrapper around the Datastore that is more reliable for the rest of the @@ -124,7 +129,19 @@ class RemoteStoreImpl implements RemoteStore { * purely based on order, and so we can just shift() writes from the front of * the writePipeline as we receive responses. */ - writePipeline: MutationBatch[] = []; + writePipeline: WritePipelineEntry[] = []; + + /** + * The operation that is enqueued to send unsent write requests enqueued in + * the write pipeline. + * + * If the `sendWriteRequestsDelayMs` argument specified to the constructor is + * `null` then this property will _never_ be set to a non-null value; + * otherwise, it will be set to a non-null value when such an operation is + * enqueued in the AsyncQueue, and will be set back to `null` once that + * operation has completed or been cancelled. + */ + sendWriteRequestsOperation: DelayedOperation | null = null; /** * A mapping of watched targets that the client cares about tracking and the @@ -168,8 +185,16 @@ class RemoteStoreImpl implements RemoteStore { readonly datastore: Datastore, readonly asyncQueue: AsyncQueue, onlineStateHandler: (onlineState: OnlineState) => void, - connectivityMonitor: ConnectivityMonitor + connectivityMonitor: ConnectivityMonitor, + readonly sendWriteRequestsDelayMs: number | null ) { + if (sendWriteRequestsDelayMs !== null) { + debugAssert( + sendWriteRequestsDelayMs > 0, + `invalid sendWriteRequestsDelayMs: ${sendWriteRequestsDelayMs} ` + + '(must be greater than zero)' + ); + } this.connectivityMonitor = connectivityMonitor; this.connectivityMonitor.addCallback((_: NetworkStatus) => { asyncQueue.enqueueAndForget(async () => { @@ -198,14 +223,16 @@ export function newRemoteStore( datastore: Datastore, asyncQueue: AsyncQueue, onlineStateHandler: (onlineState: OnlineState) => void, - connectivityMonitor: ConnectivityMonitor + connectivityMonitor: ConnectivityMonitor, + sendWriteRequestsDelayMs: number | null ): RemoteStore { return new RemoteStoreImpl( localStore, datastore, asyncQueue, onlineStateHandler, - connectivityMonitor + connectivityMonitor, + sendWriteRequestsDelayMs ); } @@ -256,6 +283,8 @@ export async function remoteStoreShutdown( ): Promise { const remoteStoreImpl = debugCast(remoteStore, RemoteStoreImpl); logDebug(LOG_TAG, 'RemoteStore shutting down.'); + remoteStoreImpl.sendWriteRequestsOperation?.cancel(); + remoteStoreImpl.sendWriteRequestsOperation = null; remoteStoreImpl.offlineCauses.add(OfflineCause.Shutdown); await disableNetworkInternal(remoteStoreImpl); remoteStoreImpl.connectivityMonitor.shutdown(); @@ -674,7 +703,7 @@ export async function fillWritePipeline( let lastBatchIdRetrieved = remoteStoreImpl.writePipeline.length > 0 ? remoteStoreImpl.writePipeline[remoteStoreImpl.writePipeline.length - 1] - .batchId + .mutationBatch.batchId : BATCHID_UNKNOWN; while (canAddToWritePipeline(remoteStoreImpl)) { @@ -732,11 +761,45 @@ function addToWritePipeline( canAddToWritePipeline(remoteStoreImpl), 'addToWritePipeline called when pipeline is full' ); - remoteStoreImpl.writePipeline.push(batch); + remoteStoreImpl.writePipeline.push({ + mutationBatch: batch, + writeRequestSent: false + }); + const writePipelineContainsOnlyUnsentWriteRequests = + remoteStoreImpl.writePipeline.every(entry => !entry.writeRequestSent); + + if ( + remoteStoreImpl.sendWriteRequestsDelayMs === null || + (!canAddToWritePipeline(remoteStoreImpl) && + writePipelineContainsOnlyUnsentWriteRequests) + ) { + remoteStoreImpl.sendWriteRequestsOperation?.cancel(); + remoteStoreImpl.sendWriteRequestsOperation = null; + sendWriteRequestsFromPipeline(remoteStoreImpl); + } else if (remoteStoreImpl.sendWriteRequestsOperation === null) { + remoteStoreImpl.sendWriteRequestsOperation = + remoteStoreImpl.asyncQueue.enqueueAfterDelay( + TimerId.RemoteStoreSendWriteRequests, + remoteStoreImpl.sendWriteRequestsDelayMs, + () => { + remoteStoreImpl.sendWriteRequestsOperation = null; + sendWriteRequestsFromPipeline(remoteStoreImpl); + return Promise.resolve(); + } + ); + } +} + +function sendWriteRequestsFromPipeline(remoteStoreImpl: RemoteStoreImpl): void { const writeStream = ensureWriteStream(remoteStoreImpl); if (writeStream.isOpen() && writeStream.handshakeComplete) { - writeStream.writeMutations(batch.mutations); + for (const pipelineEntry of remoteStoreImpl.writePipeline) { + if (!pipelineEntry.writeRequestSent) { + writeStream.writeMutations(pipelineEntry.mutationBatch.mutations); + pipelineEntry.writeRequestSent = true; + } + } } } @@ -767,8 +830,9 @@ async function onWriteHandshakeComplete( ): Promise { const writeStream = ensureWriteStream(remoteStoreImpl); // Send the write pipeline now that the stream is established. - for (const batch of remoteStoreImpl.writePipeline) { - writeStream.writeMutations(batch.mutations); + for (const pipelineEntry of remoteStoreImpl.writePipeline) { + writeStream.writeMutations(pipelineEntry.mutationBatch.mutations); + pipelineEntry.writeRequestSent = true; } } @@ -783,8 +847,12 @@ async function onMutationResult( remoteStoreImpl.writePipeline.length > 0, 'Got result for empty write pipeline' ); - const batch = remoteStoreImpl.writePipeline.shift()!; - const success = MutationBatchResult.from(batch, commitVersion, results); + const pipelineEntry = remoteStoreImpl.writePipeline.shift()!; + const success = MutationBatchResult.from( + pipelineEntry.mutationBatch, + commitVersion, + results + ); debugAssert( !!remoteStoreImpl.remoteSyncer.applySuccessfulWrite, @@ -835,7 +903,7 @@ async function handleWriteError( if (isPermanentWriteError(error.code)) { // This was a permanent error, the request itself was the problem // so it's not going to succeed if we resend it. - const batch = remoteStoreImpl.writePipeline.shift()!; + const pipelineEntry = remoteStoreImpl.writePipeline.shift()!; // In this case it's also unlikely that the server itself is melting // down -- this was just a bad request so inhibit backoff on the next @@ -847,7 +915,10 @@ async function handleWriteError( 'rejectFailedWrite() not set' ); await executeWithRecovery(remoteStoreImpl, () => - remoteStoreImpl.remoteSyncer.rejectFailedWrite!(batch.batchId, error) + remoteStoreImpl.remoteSyncer.rejectFailedWrite!( + pipelineEntry.mutationBatch.batchId, + error + ) ); // It's possible that with the completion of this mutation diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index 09171a9f038..c2faccfc31a 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -81,7 +81,15 @@ export const enum TimerId { /** * A timer used to periodically attempt index backfill. */ - IndexBackfill = 'index_backfill' + IndexBackfill = 'index_backfill', + + /** + * A timer used to send write requests for mutations to the write stream in + * the remote store. The timer is used when the remote store is configured to + * have a small delay before sending write requests to the backend so that + * multiple writes can be batched into a single HTTP request. + */ + RemoteStoreSendWriteRequests = 'remote_store_send_write_requests' } /** diff --git a/packages/firestore/test/integration/util/internal_helpers.ts b/packages/firestore/test/integration/util/internal_helpers.ts index 86ded6af3c1..558796ce836 100644 --- a/packages/firestore/test/integration/util/internal_helpers.ts +++ b/packages/firestore/test/integration/util/internal_helpers.ts @@ -61,7 +61,8 @@ export function getDefaultDatabaseInfo(): DatabaseInfo { cloneLongPollingOptions( DEFAULT_SETTINGS.experimentalLongPollingOptions ?? {} ), - /*use FetchStreams= */ false + /*use FetchStreams= */ false, + /*sendWriteRequestsDelayMs= */ null ); } diff --git a/packages/firestore/test/unit/remote/rest_connection.test.ts b/packages/firestore/test/unit/remote/rest_connection.test.ts index d45a75ce67b..216b784ecd2 100644 --- a/packages/firestore/test/unit/remote/rest_connection.test.ts +++ b/packages/firestore/test/unit/remote/rest_connection.test.ts @@ -67,7 +67,8 @@ describe('RestConnection', () => { /*forceLongPolling=*/ false, /*autoDetectLongPolling=*/ false, /*longPollingOptions=*/ {}, - /*useFetchStreams=*/ false + /*useFetchStreams=*/ false, + /*sendWriteRequestsDelayMs= */ null ); const connection = new TestRestConnection(testDatabaseInfo); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index b34421d9e0a..14dd73e1e2f 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -259,6 +259,7 @@ abstract class TestRunner { private useEagerGCForMemory: boolean; private numClients: number; private maxConcurrentLimboResolutions?: number; + private sendWriteRequestsDelayMs?: number; private databaseInfo: DatabaseInfo; protected user = User.UNAUTHENTICATED; @@ -282,7 +283,8 @@ abstract class TestRunner { /*forceLongPolling=*/ false, /*autoDetectLongPolling=*/ false, /*longPollingOptions=*/ {}, - /*useFetchStreams=*/ false + /*useFetchStreams=*/ false, + config.sendWriteRequestsDelayMs ?? null ); // TODO(mrschmidt): During client startup in `firestore_client`, we block @@ -317,7 +319,7 @@ abstract class TestRunner { initialUser: this.user, maxConcurrentLimboResolutions: this.maxConcurrentLimboResolutions ?? Number.MAX_SAFE_INTEGER - }; + } satisfies ComponentConfiguration; this.connection = new MockConnection(this.queue); @@ -1408,6 +1410,12 @@ export interface SpecConfig { * default value. */ maxConcurrentLimboResolutions?: number; + + /** + * The maximum number amount of time, in milliseconds, to delay sending + * write requests to the backend in the remote store. + */ + sendWriteRequestsDelayMs?: number; } /**