feat: Lazy-started transactions

[brettwillis]

Eliminates at least one round-trip network request for every transaction by the following

• Transactions are started lazily upon the first read request (or not at all if no reads) instead of by the initial separate beginTransaction request. All subsequent reads (and commit/rollback) are queued upon the first read operation of each transaction attempt.
• Empty transactions (no reads or writes) become a network no-op (zero requests, no begin and no commit)
• Eliminate superfluous rollback request for some types of commit errors (where the transaction is already aborted by the server)
• Added an internal getResponse API to the DocumentReader, Query and AggregateQuery classes to expose the transaction ID in the response.
• The internal _stream() APIs now emit a "transaction response" as the first element when appropriate.

Other opportunistic improvements

• Avoid initialising useless write batch and backoff instances on read-only transactions

Fixes #2015

[conventional-commit-lint-gcf]

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

[tom-andersen]

Thank you for the quite large contribution. I prefer if the rollback optimization based on error code was moved to a separate PR. I am not convinced that this change is correct. Verifying will hold up this PR. The problem is with reads having created locks, that should be released. If we wait for timeout of transaction, that can create problems for retry and other clients working on same documents.

I am giving you some preliminary feedback, but will return to do a more thorough review.

I sent you a PR to change the tests. For readTime, we do not need a transaction at all. This will eliminate the need to wait for first request to complete with transactionId, and this will also overcome potential transaction timeout.

[tom-andersen]

Overall, I really like your implementation. There are a few things I would like to see changed, but then I see no reason why this shouldn't be approved and merged.

Note, there will be a delay before we can merge since this has to fit into our upcoming release schedule. We should have this out by April.

[brettwillis]

There we go. With this last commit GitHub seems to have gotten itself unstuck. I've implemented all of those suggestions. You haven't seen the second-to-last commit Rollback is completed asynchronously, that was the one that was stuck processing.

[brettwillis]

@tom-andersen I ran this branch against a real Firestore backend and oops there were a few gotchas

• Protobuf includes zero/empty "transaction ID" buffers by default
• Sometimes the backend returns the "transaction" in the same response stream element as the "document response" and sometimes it doesn't which surprised me based on the docs. So I made it more resilient to which response comes and when.

Warrants another review sorry.

[ehsannas]

s/transactionIdOrReadTime/transactionOrReadTime

[ehsannas]

is there any value in keeping these //rollback('foo1') ?

[tom-andersen]

There is no transaction id yet, since that is part of the result from first query. So we can't rollback, even if we wanted to.

[ehsannas]

yup. I'm just saying we should remove the commented code.

[tom-andersen]

Ahh. Good point! +1

[ehsannas]

nit: to make it more consistent with the rest of this function

      return this._transactionIdPromise
                 .then(opts => resultFn.call(this, param, opts))
                 .then(r => r.result);

[MarkDuckworth]

This is fantastic. There are many nice code improvements, but it's also a significant community contribution. Thank you for your efforts.

I have a few additional comments for your consideration. But otherwise I think it looks good.

The only potential adverse affect would be if a customer is implementing parallel reads in a transaction using get(), then this has a possibility to slow the transaction if the first read is slow/large-doc. But in many cases that could be mitigated with a call to getAll().

[MarkDuckworth]

nit: Instead of this complex type definition, it might be more readable code to just construct a new type, or single object definition. Or even just keep a few individual properties.

[ehsannas]

Thanks for your contribution @brettwillis . LGTM -- only the remaining minor comments should be addressed.