Fix race condition in data provider registration

[lyonsil]

This change is 

[irahopkinson]

Reviewed 6 of 8 files at r1, all commit messages.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

cspell.json line 45 at r1 (raw file):

    "unsubscriber",
    "unsubscribers",
    "waitable"

Err... this word doesn't appear in our code. Did you use it and then remove it?

[lyonsil]

Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @irahopkinson and @lyonsil)

---

cspell.json line 45 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Err... this word doesn't appear in our code. Did you use it and then remove it?

Ah - in an earlier revision I called the new class I added "WaitableVariable". I changed it later to "AsyncVariable". "Waitable" is a real computing term that cspell doesn't seem to know about. I can remove it, though, since we're not trying to create a full computing lexicon here. 😄

[tjcouch-sil]

Interesting solution! Thanks for working hard on this.

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)

[lyonsil]

Reviewable status: 6 of 8 files reviewed, all discussions resolved (waiting on @irahopkinson and @tjcouch-sil)

---

cspell.json line 45 at r1 (raw file):

Previously, lyonsil wrote…

Ah - in an earlier revision I called the new class I added "WaitableVariable". I changed it later to "AsyncVariable". "Waitable" is a real computing term that cspell doesn't seem to know about. I can remove it, though, since we're not trying to create a full computing lexicon here. 😄

Done

[tjcouch-sil]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)

[irahopkinson]

Yep, looking good, with some minor questions.

Reviewed 1 of 8 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @lyonsil)

---

src/shared/utils/async-variable.ts line 41 at r2 (raw file):

   * @returns the promise for the value to be set
   */
  public async getPromise(): Promise<T> {

BTW This function is not async so remove it.

---

src/shared/utils/async-variable.ts line 41 at r2 (raw file):

   * @returns the promise for the value to be set
   */
  public async getPromise(): Promise<T> {

BTW Any reason for not using a getter here? I.e.:

  public get promise(): Promise<T> {
    return this.promiseToValue;
  }

---

src/shared/utils/async-variable.ts line 52 at r2 (raw file):

  public resolveToValue(value: T, throwIfAlreadySettled: boolean = false): void {
    if (this.resolver) {
      logger.debug(`${this.variableName} is being resolved now`);

I would log this after the 2 calls below (and change the message) to reduce the chance of race conditions.

---

src/shared/utils/async-variable.ts line 68 at r2 (raw file):

  public rejectWithReason(reason: string, throwIfAlreadySettled: boolean = false): void {
    if (this.rejecter) {
      logger.debug(`${this.variableName} is being rejected now`);

I would log this after the 2 calls below (and change the message) to reduce the chance of race conditions.

---

src/shared/utils/async-variable.ts line 81 at r2 (raw file):

   * @returns whether the variable was already resolved or rejected
   */
  public hasSettled(): boolean {

BTW I would also make this a getter.

---

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

  const TEST_VALUE: string = 'A1B2C3';
  const testVariable: AsyncVariable<string> = new AsyncVariable<string>(VARIABLE_NAME);
  (async () => {

I feel like you could remove some of the complexity here (2x async self calling functions) by making the test arrow function to be aysnc, however that is only a guess.

[lyonsil]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @irahopkinson and @lyonsil)

---

src/shared/utils/async-variable.ts line 41 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW This function is not async so remove it.

Good point - done

---

src/shared/utils/async-variable.ts line 41 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW Any reason for not using a getter here? I.e.:

  public get promise(): Promise<T> {
    return this.promiseToValue;
  }

ok - changed to a getter

---

src/shared/utils/async-variable.ts line 52 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I would log this after the 2 calls below (and change the message) to reduce the chance of race conditions.

I put the log message where I did because if I were debugging something related to this, I'd want to find the "is being resolved now" message and look right after that to see what happens. If I put the message later, then if something went wrong with the code that was waiting on the promise, the debug message might never print. I assume nothing really latency sensitive would want to write code using one of these. Instead this class provides a clear synchronization mechanism between tasks in a way that avoids polling.

---

src/shared/utils/async-variable.ts line 68 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I would log this after the 2 calls below (and change the message) to reduce the chance of race conditions.

Same as above

---

src/shared/utils/async-variable.ts line 81 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW I would also make this a getter.

ok - changed to a getter

---

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I feel like you could remove some of the complexity here (2x async self calling functions) by making the test arrow function to be aysnc, however that is only a guess.

I thought the expected way the new class would be used is clear the way this test is currently written. Can you provide a code snippet of how you think it would be clearer to demonstrate how to use it? I'm not sure I follow what you have in mind.

[irahopkinson]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

src/shared/utils/async-variable.ts line 81 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

ok - changed to a getter

Great. To follow our boolean naming conventions leave the name as hasSettled.

---

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I thought the expected way the new class would be used is clear the way this test is currently written. Can you provide a code snippet of how you think it would be clearer to demonstrate how to use it? I'm not sure I follow what you have in mind.

Like I said, I was only guessing, so I thought I'd better try it myself since I opened my big mouth :). BTW normally tests are in 3 parts: setup; SUT; checks. These 3 are separated below with new lines.

test('wait on other async tasks to set and notify variables', () => {
  const VARIABLE_NAME: string = 'abc';
  const TEST_VALUE: string = 'A1B2C3';
  const testVariable: AsyncVariable<string> = new AsyncVariable<string>(VARIABLE_NAME);
  (async () => {
    await expect(testVariable.promise).resolves.toBe(TEST_VALUE);
  })();
  expect(testVariable.settled).toBeFalsy();

  testVariable.resolveToValue(TEST_VALUE);

  expect(testVariable.settled).toBeTruthy();
});

[lyonsil]

Reviewable status: 5 of 8 files reviewed, all discussions resolved (waiting on @irahopkinson)

---

src/shared/utils/async-variable.ts line 81 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Great. To follow our boolean naming conventions leave the name as hasSettled.

Done

---

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Like I said, I was only guessing, so I thought I'd better try it myself since I opened my big mouth :). BTW normally tests are in 3 parts: setup; SUT; checks. These 3 are separated below with new lines.

test('wait on other async tasks to set and notify variables', () => {
  const VARIABLE_NAME: string = 'abc';
  const TEST_VALUE: string = 'A1B2C3';
  const testVariable: AsyncVariable<string> = new AsyncVariable<string>(VARIABLE_NAME);
  (async () => {
    await expect(testVariable.promise).resolves.toBe(TEST_VALUE);
  })();
  expect(testVariable.settled).toBeFalsy();

  testVariable.resolveToValue(TEST_VALUE);

  expect(testVariable.settled).toBeTruthy();
});

I pulled the 3 lines out of the async function as you did in your snippet.

In this test, it is important that all of the expect lines are correct. There is a series of steps being followed, and all checks can't be held to the end of the test.

I consider tests to be 2 things:

1. Verification that code is working as expected, and
2. A form of "live documentation" for maintainers and maybe users.

Sometimes I've found that if one is stick tightly to some testing rules (e.g., no more than 1 assert/expect/validation per test), it requires creating so many different tests with similar code that the tests cease to be as useful of "live documentation." In cases like this, I think it's helpful to be able to assert/expect/validate/etc. a few related things in a single test so that you aren't repeating the same concepts/steps in multiple tests.

I don't think you're suggesting that I break up this test into multiple parts, but I think perhaps you and I are starting from slightly different places. That's why I laid out my thoughts in more detail here.

[irahopkinson]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I pulled the 3 lines out of the async function as you did in your snippet.

In this test, it is important that all of the expect lines are correct. There is a series of steps being followed, and all checks can't be held to the end of the test.

I consider tests to be 2 things:

1. Verification that code is working as expected, and
2. A form of "live documentation" for maintainers and maybe users.

Sometimes I've found that if one is stick tightly to some testing rules (e.g., no more than 1 assert/expect/validation per test), it requires creating so many different tests with similar code that the tests cease to be as useful of "live documentation." In cases like this, I think it's helpful to be able to assert/expect/validate/etc. a few related things in a single test so that you aren't repeating the same concepts/steps in multiple tests.

I don't think you're suggesting that I break up this test into multiple parts, but I think perhaps you and I are starting from slightly different places. That's why I laid out my thoughts in more detail here.

I agree with everything you said here.

---

src/shared/utils/async-variable.test.ts line 22 at r4 (raw file):

    await expect(testVariable.promise).rejects.toMatch(TEST_REASON);
  })();
  (async () => {

BTW can remove this self-calling async also.

[lyonsil]

Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @irahopkinson)

---

src/shared/utils/async-variable.test.ts line 22 at r4 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW can remove this self-calling async also.

yep, good point

[irahopkinson]

Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @lyonsil)

---

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I agree with everything you said here.

I was just saying it's helpful to mark the SUT (software under test) in some way. New lines like I did can do it - a comment would also work:

// SUT
testVariable.resolveToValue(TEST\_VALUE);

[irahopkinson]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)

[lyonsil]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @lyonsil)

---

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I was just saying it's helpful to mark the SUT (software under test) in some way. New lines like I did can do it - a comment would also work:

// SUT
testVariable.resolveToValue(TEST\_VALUE);

Well, this is also being tested:

await expect(testVariable.promise).resolves.toBe(TEST\_VALUE);

Is there a pattern the team is trying to follow to call out things like this? I don't remember ever seeing something like this in tests in the past (here or elsewhere).

Now that I look back at papi-util.test.ts, I see there are extra spaces there that I didn't understand. Perhaps that is what you had in mind, but I didn't realize that until now.

Perhaps we should have a section on tests in the style guide and review it as a group?

[irahopkinson]

src/shared/utils/async-variable.test.ts line 7 at r2 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Well, this is also being tested:

await expect(testVariable.promise).resolves.toBe(TEST\_VALUE);

Is there a pattern the team is trying to follow to call out things like this? I don't remember ever seeing something like this in tests in the past (here or elsewhere).

Now that I look back at papi-util.test.ts, I see there are extra spaces there that I didn't understand. Perhaps that is what you had in mind, but I didn't realize that until now.

Perhaps we should have a section on tests in the style guide and review it as a group?

To me that is not the SUT call, it's a check (expect to resolve) that happens at the end but is actually setup at the beginning in the setup section to check at the end.

Yes, adding a section in the style guide would be a good idea.