Infinite loop after last unsubscribe in withFilter function.

[swangee]

I use withFilter to validate payload and got an infinite loop when there are no subscribers left (payload is resolved immediately with {value: undefined, done: true}).

const getNextPromise = () => {
        return asyncIterator
          .next()
          .then(payload => Promise.all([
            payload,
            Promise.resolve(filterFn(payload.value, args, context, info)).catch(() => false),
          ]))
          .then(([payload, filterResult]) => {
            if (filterResult === true) {
              return payload;
            }

            // Skip the current value and wait for the next one
            return getNextPromise();
          });
      };

This function recursively calls itself even if payload.done is true and it leads to infinite call. Adding check for payload.done before return getNextPromise(); fixes this problem or am I missing something?

[maktouch]

Doing if (filterResult === true || payload.done === true) fixes it for me.

    const getNextPromise = () => {
        return asyncIterator
          .next()
          .then(payload => Promise.all([
            payload,
            Promise.resolve(filterFn(payload.value, args, context, info)).catch(() => false),
          ]))
          .then(([payload, filterResult]) => {
            if (filterResult === true || payload.done === true) {
              return payload;
            }

            // Skip the current value and wait for the next one
            return getNextPromise();
          });
      };

[cncolder]

+1 Fall into this trap

[swangee]

@maktouch Firstly if fixed it in the same way, but it has a side effect - undefined is passed to filter function once, so check for undefined is needed in a filterFn.
So I decided it add some inner state to withFilter with flag returned and added the check to it. Now it looks in this way:

module.exports.withFilter = (asyncIteratorFn, filterFn) => {
    return (rootValue, args, context, info) => {
      const asyncIterator = asyncIteratorFn();

      let returned = false;

      const getNextPromise = () => {
        if (returned) {
          return {done: true, value: undefined};
        }

        return asyncIterator
          .next()
          .then(payload => Promise.all([
            payload,
            returned ? false : Promise.resolve(filterFn(payload.value, args, context, info)).catch(() => false),
          ]))
          .then(([payload, filterResult]) => {
            if (filterResult === true) {
              return payload;
            }

            // Skip the current value and wait for the next one
            return getNextPromise();
          });
      };

      return {
        next() {
          return getNextPromise();
        },
        return() {
          returned = true;
          return asyncIterator.return();
        },
        throw(error) {
          return asyncIterator.throw(error);
        },
        [$$asyncIterator]() {
          return this;
        },
      };
    };
};

[cncolder]

@maktouch What about only skip filterFn if value is undefined?

Promise.resolve(!!payload.value && filterFn(payload.value, args, context, info)).catch(() => false)

[maktouch]

I don't know, actually. I'm not sure what's the intended behaviour of getNextPromise; I noticed that my subscribe functions gets called every time I subscribe to something, and it's passing undefined. It would be nice to have input from the maintainers for some more clarification.

In the meantime, I'm checking for payload === undefined in the filterFn

[green-coder]

.. so @cncolder and @vedebel actually saw the bug #132 coming but it was eventually merged into the codebase. :-(