queued transaction can be also rollbacked

[jardakotesovec]

This is follow up to #607 that adds posibility to rollback transaction with status COMMIT_QUEUED.

There is one more missing piece for this use case that I was not sure how to approach.

To replace transaction that is queued I need to first check if its status is still COMMIT_QUEUED. Issue is that transaction is removed from pendingTransactionMap once its successfully committed. So I can easily end up with exception from getStatus(), which is bit overreaction :-).

So maybe return some status like FINISHED instead of throwing if its not in pending queue? I am of course very opened about naming :-).

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[xuorig]

nit: use let here and for index :)

[jardakotesovec]

Can I ask for feedback here?

[josephsavona]

if (collisionKey && collisionQueue) {

[yungsters]

Per @josephsavona, doing the check here makes it so you don't have to do it on line 144. Better yet, you can avoid the redundancy by doing:

if (collisionKey) {
  const collisionQueue = this._collisionQueueMap[collisionKey];
  if (collisionQueue) {
    // Flow now knows that both `collisionKey` and `collisionQueue` are truthy.
  }
}

[yungsters]

@jardakotesovec Thanks for your contribution. I definitely want to see this fix land, but I have some feedback.

[yungsters]

To reduce the number of code paths that rollback follows (which just makes the code slightly easier to reason about), what do you think about moving all of this logic into _clearPendingTransaction?

[jardakotesovec]

@yungsters Thanks a lot for feedback.
collisionQueue is managed in commit, _advanceCollisionQueue, _failCollisionQueue. So If I move it to _clearPendingTransaction than both _failCollisionQueue and _clearPendingTransaction will try to remove it for certain code path (its necessary to slice() queue in _failCollisionQueue to iterate it correctly) from the collisionQueue.

And its not possible just move this responsibility only to _clearPendingTransaction since its necessary to clean collisionQueue also for situations when its commited again (not rollbacked).

But if you still prefer it I will move it there and adjust _failCollisionQueue to deal with that by using slice and iterating on copy:

  _failCollisionQueue(transaction: PendingTransaction): void {
    const collisionKey = transaction.getCollisionKey();
    if (collisionKey) {
      const collisionQueue = nullthrows(this._collisionQueueMap[collisionKey]);
      // Remove the transaction that called this function.
      collisionQueue.shift();
      collisionQueue.slice().forEach(
        transaction => this._handleCommitFailure(transaction, null)
      );
      delete this._collisionQueueMap[collisionKey];
    }
  }

[yungsters]

@jardakotesovec Ahh... makes sense. You clearly understand this code better than I do, haha. I'll test this internally and land it. Thanks again!

[yungsters]

@facebook-github-bot import

[jardakotesovec]

@yungsters Thanks a lot. Do you have some thoughts on getStatus issue I mentioned in my initial comment? Would be handy if getStatus would return something when its processed instead of exception.

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[yungsters]

Do you have some thoughts on getStatus issue I mentioned in my initial comment? Would be handy if getStatus would return something when its processed instead of exception.

Indeed, but the difficultly is in differentiating between completed and invalid IDs. One solution would be to keep around the map of IDs that have been successfully committed, but that will continue to leak memory into the future.

Another solution would be to change RelayMutationQueue to expose methods on some instance that callers can obtain a reference for as long as needed.

@josephsavona Do you remember the reason for why we chose to use IDs instead of exposing a public form of the pending transaction (which would have all the methods like getError, getStatus, commit, etc.)?