medikoo / memoizee

Complete memoize/cache solution for JavaScript
ISC License
1.73k stars 61 forks source link

Rejected Promise stays in Cache #93

Closed john681611 closed 6 years ago

john681611 commented 6 years ago

We seem to be finding that rejected promises are still being stored in the cache. Here is the test we are using. Modules used: Mocha,Sinon,Chai,Chai-as-promised;

it.only('should not cache rejected promises', function (done) {
        // Given
        const memoizee = require('memoizee');
        let defaultResult = 1;
        let stub = sinon.stub().returns(Promise.reject(new Error('Bang!')));
        let memoized = memoizee(stub, { promise: true });

        // When
        expect(memoized()).to.be.rejectedWith(Error);

        stub.returns(Promise.resolve(defaultResult));

        // Then
        let result = memoized();
        expect(stub).to.have.been.calledTwice;
        expect(result).to.eventually.equal(defaultResult).notify(done);
    });

Output:

expected stub to have been called exactly twice, but it was called once stub() => [ZoneAwarePromise] { zone_symbolstate: false, zone_symbolvalue: Error: Bang! } at memoized (/Users/c2444781/Documents/mns-fe-foundation/node_modules/memoizee/lib/configure-map.js:81:23) AssertionError: expected stub to have been called exactly twice, but it was called once stub() => [ZoneAwarePromise] { zone_symbolstate: false, zone_symbolvalue: Error: Bang! } at memoized (node_modules/memoizee/lib/configure-map.js:81:23) at Context. (src/components/memoize/memoize.test.js:219:34)

medikoo commented 6 years ago

@john681611 rejected promise is released from cache after rejection is discovered. Here you run second test before rejection is discovered.

It all comes down to design of promises. You're not capable to get value of a promise synchronously, but earliest in next event loop. Therefore even if memoized function immediately resolves with rejected promise, the memoizee internals can learn about it earliest in next event loop.

Following test case should pass for you:

it.only('should not cache rejected promises', function (done) {
  // Given
  const memoizee = require('memoizee');
  let defaultResult = 1;
  let stub = sinon.stub().returns(Promise.reject(new Error('Bang!')));
  let memoized = memoizee(stub, { promise: true });

  // When
  expect(memoized()).to.be.rejectedWith(Error);

  stub.returns(Promise.resolve(defaultResult));

  // Then
  setTimeout(() => {
    let result = memoized();
    expect(stub).to.have.been.calledTwice;
    expect(result).to.eventually.equal(defaultResult).notify(done);
  });
});
john681611 commented 6 years ago

The code in the setTimout never fires

medikoo commented 6 years ago

The code in the setTimeout never fires

Then there must be some issue with your setup. It's hard for me to see why setTimeout doesn't work on your side. You may prepare some reproducible test case on codepen or jsbin, from that we can move forward