feat: enable retry for addItem and add more debug info

[raymondfeng]

Follow up #13 per @bajtos comments:

• Enable retry for adding items to a shopping cart
• Add more debug statements

[b-admike]

Do we want package-lock.json in this repository?

[raymondfeng]

Yes, package.lock is good for an application to make it repeatable. For example, we can use npm ci and npm audit fix.

[b-admike]

nitpick: Fail to -> Failed to

[raymondfeng]

I'm fine with either tense.

[b-admike]

do we want tests for this function?

[raymondfeng]

We could add a unit test but not sure how to test it with redis.

[raymondfeng]

Added tests.

[bajtos]

I find these positional arguments difficult to understand. What is 10 and 100?

I am proposing to change retry API to accept an options object instead of positional parameters.

    return retry<ShoppingCart>(
       {
         run: () => this.shoppingCartRepository.addItem(userId, item),
         description: `update the shopping cart for '${userId}'`,
       },
       {
         maxRetries: 10,
         retryInterval: 100, 
       });

[bajtos]

I find this sentence ambiguous. If the transaction fails, is the function returning null or a Promise that's resolved to null?

Also: A transaction can fail for other reasons besides a race condition, for example the server can become unreachable or unresponsive. We are not returning null but rejecting the promise in those cases!

Proposed wording:

A promise that's resolved with the updated ShoppingCart instance or with null if the transaction failed because of a race condition.

[bajtos]

KISS & YAGNI.

Let's remove isDone from this pull request since we don't need it (yet).

[bajtos]

IMO, it should be the responsibility of a repository to deal with the need to retry this transaction. To me, it's an implementation detail specific to Redis storage. If we use a different backend that supports atomic partial updates, then retry won't be necessary.

It feels very wrong to me that we have to change controller code when the backing database is changed. The repository should be the part of the application dealing with database complexities.

Please move retry to ShoppingCartRepository.

[bajtos]

Assuming we want to make retry generic: I think it's not safe to assume that null/undefined is always an invalid result signaling a need to retry.

I think we should remove result != null check and modify the shopping-cart code calling retry to provide isDone function too. (In which case my previous comment #14 (comment) should be ignored.)

Alternatively, it may be better & easier to combine these two function into one and change the return value into an object with two properties: done: boolean and value: T.

• {done: true, value: /*result*/} - the task finished successfully
• {done: false /* value can be omitted */} - a retry is needed

One more important point: your current implementation retries regardless of whether the transaction was successfully executed with rollback result or whether there was an error which may prevent retries from succeeding (the commands are not valid, the server is not responding, etc.). That does not feel right to me, as it can retry a request that's known to fail again, unnecessarily putting more pressure on the backend system.

IMO, this retry function should fail immediately when the task fails. Implementing a circuit-switcher behavior is out of scope of this work.

[bajtos]

Data builders typically use Object.assign, allowing users to leverage sensible defaults provided by the data builder and customize only the properties relevant to the test.

const defaultData = {
 // sensible property values
};
return new ShoppingCartItem(
  Object.assign(defaultData, item)
);

[bajtos]

Just use givenAnItem(), the specific property values are not important here.

[bajtos]

Please move promisify(setTimeout) out of this function, so that we don't have to create a new function whenever sleep is called.

const setTimeoutAsync = promisify(setTimeout);

export function sleep(ms) {
  return setTimeoutAsync(ms);
}

I think this can be further simplified as follows:

export const sleep = promisify(setTimeout);

[raymondfeng]

@bajtos Great feedback. PTAL.

[bajtos]

IIUC, check is both signaling whether the task is finished and converting the outcome of run to the final value. That seems overly complicated to me, and at the same time not flexible enough, because check has T both on input and output, thus it cannot do that much.

In my original comment, I had the following interface in my mind:

export interface TaskFinished<T> {
  done: true;
  result: T;
}

export interface RetryNeeded {
  done: false;
}

export interface Task<T> {
  run(): Promise<TaskFinished<T> | RetryNeeded>;
  description?: string;
}

On the second look, I see that your check method is accepting also err that may have been returned by run. What benefits do you see in this design?

I would personally prefer to use regular async/await and try/catch flow for error handling, instead of having to use run/check semantics.

run() {
  try {
    // do some work
    return {done: true, value: 'some data'};
  } catch (err) {
    // if the error means I should retry
      return {done: false}
    else throw err;
  }
}

Thoughts?

[raymondfeng]

@bajtos PTAL

[bajtos]

Nitpick: this type allows the developer to provide combinations of done/value that are not valid:

• {done: true} (Missing value. What should be the outcome of the operation?)
• {done: false, value: 'some data'}

[bajtos]

status.value!

Please don't! The typings are designed in such way that task.run() can return a value that's undefined.

If you want to preserve TaskStatus interface then you need to change retry to return T | null and push the responsibility of handling null/undefined result to callers of retry. This is not a great UX to me.

I think a better solution is to follow my proposal outlined in #14 (comment), I believe it provides information to allow TypeScript understand that when status.done is true, then status.value must be defined.

export interface TaskFinished<T> {
  done: true;
  result: T;
}

export interface RetryNeeded {
  done: false;
}

export interface Task<T> {
  run(): Promise<TaskFinished<T> | RetryNeeded>;
  description?: string;
}

[bajtos]

Nitpick: Since retry is not used at REST layer, but inside a repository, I think we shouldn't be throwing an HttpError and use err.code to provide a machine-readable flag that can be used to detect this particular error.

[bajtos]

The current implementation of retry is good enough for the specific use case of our shopping example.

My last comments above would become important if we wanted to make retry more generic and move it to loopback-next.