Skip to content

Add onCommitted listeners for transactions #2265

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

Merged
merged 5 commits into from
Oct 11, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
31 changes: 16 additions & 15 deletions packages/firestore/src/local/indexeddb_persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -758,12 +758,17 @@ export class IndexedDbPersistence implements Persistence {
? 'readwrite-idempotent'
: 'readwrite';

let persistenceTransaction: PersistenceTransaction;

// Do all transactions as readwrite against all object stores, since we
// are the only reader/writer.
return this.simpleDb.runTransaction(
simpleDbMode,
ALL_STORES,
simpleDbTxn => {
return this.simpleDb
.runTransaction(simpleDbMode, ALL_STORES, simpleDbTxn => {
persistenceTransaction = new IndexedDbTransaction(
simpleDbTxn,
this.listenSequence.next()
);

if (mode === 'readwrite-primary') {
// While we merely verify that we have (or can acquire) the lease
// immediately, we wait to extend the primary lease until after
Expand All @@ -785,12 +790,7 @@ export class IndexedDbPersistence implements Persistence {
PRIMARY_LEASE_LOST_ERROR_MSG
);
}
return transactionOperation(
new IndexedDbTransaction(
simpleDbTxn,
this.listenSequence.next()
)
);
return transactionOperation(persistenceTransaction);
})
.next(result => {
return this.acquireOrExtendPrimaryLease(simpleDbTxn).next(
Expand All @@ -799,13 +799,14 @@ export class IndexedDbPersistence implements Persistence {
});
} else {
return this.verifyAllowTabSynchronization(simpleDbTxn).next(() =>
transactionOperation(
new IndexedDbTransaction(simpleDbTxn, this.listenSequence.next())
)
transactionOperation(persistenceTransaction)
);
}
}
);
})
.then(result => {
persistenceTransaction.raiseOnCommittedEvent();
return result;
});
}

/**
Expand Down
7 changes: 5 additions & 2 deletions packages/firestore/src/local/memory_persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ export class MemoryPersistence implements Persistence {
this.referenceDelegate.onTransactionStarted();
return transactionOperation(txn)
.next(result => {
txn.raiseOnCommittedEvent();
Copy link
Contributor

Choose a reason for hiding this comment

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

This ordering seems off. Couldn't the reference delegate conceivably modify the transaction?

IndexedDbPersistence has the better implementation. Chaining this off the promise makes it clearer that it's altogether outside the transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to invoke the committed handler as the last step.

return this.referenceDelegate
.onTransactionCommitted(txn)
.next(() => result);
Expand All @@ -203,8 +204,10 @@ export class MemoryPersistence implements Persistence {
* Memory persistence is not actually transactional, but future implementations
* may have transaction-scoped state.
*/
export class MemoryTransaction implements PersistenceTransaction {
constructor(readonly currentSequenceNumber: ListenSequenceNumber) {}
export class MemoryTransaction extends PersistenceTransaction {
constructor(readonly currentSequenceNumber: ListenSequenceNumber) {
super();
}
}

export class MemoryEagerDelegate implements ReferenceDelegate {
Expand Down
13 changes: 12 additions & 1 deletion packages/firestore/src/local/persistence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,25 @@ import { RemoteDocumentCache } from './remote_document_cache';
import { ClientId } from './shared_client_state';

/**
* Opaque interface representing a persistence transaction.
* A base class representing a persistence transaction, encapsulating both the
* transaction's sequence numbers as well as a list of onCommitted listeners.
*
* When you call Persistence.runTransaction(), it will create a transaction and
* pass it to your callback. You then pass it to any method that operates
* on persistence.
*/
export abstract class PersistenceTransaction {
private readonly onCommittedListeners: Array<() => {}> = [];

abstract readonly currentSequenceNumber: ListenSequenceNumber;

addOnCommittedListener(listener: () => {}): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we require that the committed listeners return void? That way you can't accidentally have a return statement in your callback that you intend for the outer method.

Or would that interfere with using short arrow functions in the callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized this when I started to modify my "applyRemoteEvent" PR. It is much better if we return void.

this.onCommittedListeners.push(listener);
}

raiseOnCommittedEvent(): void {
this.onCommittedListeners.forEach(listener => listener());
}
}

/** The different modes supported by `IndexedDbPersistence.runTransaction()`. */
Expand Down