@Linkgoron @benjamingr

If I understand this correctly, this doesn't exactly test what I wanted to check. I wanted to check the following:

After 2 iterations, delay the current iteration long enough for the interval to create another value. Then, abort the controller, and see that the value that was created during the previous delay will be read. I believe that this will abort the controller only after the third iteration has already started.

If I understand this correctly, this doesn't exactly test what I wanted to check. I wanted to check the following:

After 2 iterations, delay the current iteration long enough for the interval to create another value. Then, abort the controller, and see that the value that was created during the previous delay will be read. I believe that this will abort the controller only after the third iteration has already started.

@Linkgoron Ah, I see. OK, I've modified it to use that behavior. Please take a look. I tested it with -j95 --repeat=192 and it passed 100% of the time.

CI: https://ci.nodejs.org/job/node-test-pull-request/35974/

CI: https://ci.nodejs.org/job/node-test-pull-request/35978/

If I understand this correctly, this doesn't exactly test what I wanted to check. I wanted to check the following:
After 2 iterations, delay the current iteration long enough for the interval to create another value. Then, abort the controller, and see that the value that was created during the previous delay will be read. I believe that this will abort the controller only after the third iteration has already started.

@Linkgoron Ah, I see. OK, I've modified it to use that behavior. Please take a look. I tested it with -j95 --repeat=192 and it passed 100% of the time.

I still think that it isn't exactly what the original test tested. The test is supposed to check that if a value was generated before the controller was aborted and wasn't read yet ("backpressure"), the following call to .next returns the last generated value instead of throwing. If I understand this correctly, the event is fired after the the third iteration has already begun.

The test is supposed to check that if a value was generated before the controller was aborted and wasn't read yet ("backpressure"), the following call to .next returns the last generated value instead of throwing.

@Linkgoron When you say "a value was generated", what do you mean? I thought it meant a value had been generated by the for await. Is that wrong?

To check what's going on here, I added a logging variable foo like this:

{
  let foo = '';
  async function runInterval(fn, intervalTime, signal, emitter) {
    const input = 'foobar';
    const interval = setInterval(intervalTime, input, { signal });
    let iteration = 0;
    foo += 'entering for loop\n';
    for await (const value of interval) {
      foo += `got value ${value} from for await\n`;
      if (emitter) {
        // Next line uses `iteration + 1` because we haven't incremented `iteration` yet.
        foo += `emitting the event from iteration ${iteration + 1}\n`;
        emitter.emit('myevent');
      }
      assert.strictEqual(value, input);
      iteration++;
      foo += `about to await the passed callback for iteration ${iteration}\n`;
      await fn(iteration);
      foo += `finished awaiting the passed callback for iteration ${iteration}\n`;
    }
  }

  {
    // Check that if we abort when we have some unresolved callbacks,
    // we actually call them.
    const controller = new AbortController();
    const myEvent = new EventEmitter();
    const { signal } = controller;
    const delay = 10;
    let totalIterations = 0;
    const timeoutLoop = runInterval(async (iterationNumber) => {
      if (iterationNumber <= 2) {
        assert.strictEqual(signal.aborted, false);
      }
      if (iterationNumber === 2) {
        foo += 'adding the once listener in iteration 2\n';
        myEvent.once('myevent', () => {
          foo += 'aborting\n';
          controller.abort();
          foo += 'aborted\n';
        });
      }
      if (iterationNumber > 2) {
        assert.strictEqual(signal.aborted, true);
      }
      if (iterationNumber > totalIterations) {
        totalIterations = iterationNumber;
      }
    }, delay, signal, myEvent);

    timeoutLoop.catch(common.mustCall(() => {
      console.log(foo);
      assert.ok(totalIterations >= 3, `iterations was ${totalIterations} < 3`);
    }));
  }
}

The results were this:

entering for loop
got value foobar from for await
emitting the event from iteration 1
about to await the passed callback for iteration 1
finished awaiting the passed callback for iteration 1
got value foobar from for await
emitting the event from iteration 2
about to await the passed callback for iteration 2
adding the once listener in iteration 2
finished awaiting the passed callback for iteration 2
got value foobar from for await
emitting the event from iteration 3
aborting
aborted
about to await the passed callback for iteration 3
finished awaiting the passed callback for iteration 3

If I'm understanding correctly, then this appears to be doing the right thing--specifically, this at the end:

got value foobar from for await
emitting the event from iteration 3
aborting
aborted
about to await the passed callback for iteration 3
finished awaiting the passed callback for iteration 3

And if I"m not understanding correctly, are you able to see where I'm getting it wrong?

@Trott

@Linkgoron When you say "a value was generated", what do you mean? I thought it meant a value had been generated by the for await. Is that wrong?

Oh, I see the confusion. When I say that a value is generated, I'm talking about the setInterval that's ״running״ in the background "in" the generator. Every time that it's "invoked" (every delay ms) it "generates" a new value (and there might be a few waiting because of "backpressure"). The way I see it, thefor await doesn't "generate" values, it just waits for values to be generated by the generator (and if there's backpressure, it just waits until the next tick).

And if I"m not understanding correctly, are you able to see where I'm getting it wrong?

The case that the original test was trying to show was what happens if the .next is invoked after the generator had backpressure, and was also aborted. That's why it needed to get aborted before the for await tried to get the third value, but the controller.abort() itself had to happen after the internal setInterval already "generated" the third value.

So, generate the 2nd value -> wait for delay ms, long enough for the internal interval to get executed again -> abort the controller -> see that the 3rd value is obtained by next (instead of getting an AbortError).

Oh, I see the confusion. When I say that a value is generated, I'm talking about the setInterval that's ״running״ in the background "in" the generator. Every time that it's "invoked" (every delay ms) it "generates" a new value (and there might be a few waiting because of "backpressure"). The way I see it, thefor await doesn't "generate" values, it just waits for values to be generated by the generator (and if there's backpressure, it just waits until the next tick).

Ahhhhhh.... I think I see now. In that case, probably bumping the setTimer() to delay * 4 instead of delay * 2 will fix it. Uh, even though I said before that probably wasn't the way to go. 🙃 There may yet be a better way, but that should work. (See current change.)

CI: https://ci.nodejs.org/job/node-test-pull-request/35998/

Oh, I see the confusion. When I say that a value is generated, I'm talking about the setInterval that's ״running״ in the background "in" the generator. Every time that it's "invoked" (every delay ms) it "generates" a new value (and there might be a few waiting because of "backpressure"). The way I see it, thefor await doesn't "generate" values, it just waits for values to be generated by the generator (and if there's backpressure, it just waits until the next tick).

Ahhhhhh.... I think I see now. In that case, probably bumping the setTimer() to delay * 4 instead of delay * 2 will fix it. Uh, even though I said before that probably wasn't the way to go. 🙃 There may yet be a better way, but that should work. (See current change.)

Yes, that'll work, although you could also do await setTimeout(delay); twice before the controller.abort() (in the if)

CI: https://ci.nodejs.org/job/node-test-pull-request/36002/ ✅

@nodejs/testing @nodejs/timers This needs a review.

Landed in b4264b5