fix(context): cache binding value or promise as-is to avoid racing co…

…ndition Fixes #5730
loopbackio · Jun 15, 2020 · 38b9b96 · 38b9b96
1 parent c971347
commit 38b9b96
Show file tree

Hide file tree

Showing 2 changed files with 101 additions and 13 deletions.
diff --git a/packages/context/src/__tests__/unit/resolver.unit.ts b/packages/context/src/__tests__/unit/resolver.unit.ts
@@ -6,12 +6,14 @@
 import {expect} from '@loopback/testlab';
 import {
   BindingAddress,
+  BindingScope,
   Context,
   Getter,
   inject,
   Injection,
   instantiateClass,
   invokeMethod,
+  Provider,
   ResolutionSession,
 } from '../..';
 
@@ -735,3 +737,91 @@ describe('async method injection', () => {
     expect(inst.bar).to.eql('FOO');
   });
 });
+
+describe('concurrent resolutions', () => {
+  let asyncCount = 0;
+  let syncCount = 0;
+
+  beforeEach(() => {
+    asyncCount = 0;
+    syncCount = 0;
+  });
+
+  it('returns the same value for concurrent resolutions of the same binding - CONTEXT', async () => {
+    const ctx = new Context('request');
+
+    ctx
+      .bind('asyncValue')
+      .toProvider(AsyncValueProvider)
+      .inScope(BindingScope.CONTEXT);
+    ctx
+      .bind('syncValue')
+      .toProvider(SyncValueProvider)
+      .inScope(BindingScope.CONTEXT);
+    ctx.bind('AsyncValueUser').toClass(AsyncValueUser);
+    const user: AsyncValueUser = await ctx.get('AsyncValueUser');
+    expect(user.asyncValue1).to.eql('async value: 0');
+    expect(user.asyncValue2).to.eql('async value: 0');
+    expect(user.syncValue1).to.eql('sync value: 0');
+    expect(user.syncValue2).to.eql('sync value: 0');
+  });
+
+  it('returns the same value for concurrent resolutions of the same binding -  SINGLETON', async () => {
+    const ctx = new Context('request');
+
+    ctx
+      .bind('asyncValue')
+      .toProvider(AsyncValueProvider)
+      .inScope(BindingScope.SINGLETON);
+    ctx
+      .bind('syncValue')
+      .toProvider(SyncValueProvider)
+      .inScope(BindingScope.SINGLETON);
+    ctx.bind('AsyncValueUser').toClass(AsyncValueUser);
+    const user: AsyncValueUser = await ctx.get('AsyncValueUser');
+    expect(user.asyncValue1).to.eql('async value: 0');
+    expect(user.asyncValue2).to.eql('async value: 0');
+    expect(user.syncValue1).to.eql('sync value: 0');
+    expect(user.syncValue2).to.eql('sync value: 0');
+  });
+
+  it('returns new values for concurrent resolutions of the same binding -  TRANSIENT', async () => {
+    const ctx = new Context('request');
+
+    ctx
+      .bind('asyncValue')
+      .toProvider(AsyncValueProvider)
+      .inScope(BindingScope.TRANSIENT);
+    ctx
+      .bind('syncValue')
+      .toProvider(SyncValueProvider)
+      .inScope(BindingScope.TRANSIENT);
+    ctx.bind('AsyncValueUser').toClass(AsyncValueUser);
+    const user: AsyncValueUser = await ctx.get('AsyncValueUser');
+    expect(user.asyncValue1).to.eql('async value: 0');
+    expect(user.asyncValue2).to.eql('async value: 1');
+    expect(user.syncValue1).to.eql('sync value: 0');
+    expect(user.syncValue2).to.eql('sync value: 1');
+  });
+
+  class AsyncValueProvider implements Provider<string> {
+    public value() {
+      return Promise.resolve(`async value: ${asyncCount++}`);
+    }
+  }
+
+  class SyncValueProvider implements Provider<string> {
+    public value() {
+      return `sync value: ${syncCount++}`;
+    }
+  }
+
+  class AsyncValueUser {
+    constructor(
+      @inject('asyncValue') readonly asyncValue1: string,
+      @inject('asyncValue') readonly asyncValue2: string,
+      @inject('syncValue') readonly syncValue1: string,
+      @inject('syncValue') readonly syncValue2: string,
+    ) {}
+  }
+});
diff --git a/packages/context/src/binding.ts b/packages/context/src/binding.ts
@@ -310,7 +310,7 @@ export class Binding<T = BoundValue> extends EventEmitter {
     return this._source?.type;
   }
 
-  private _cache: WeakMap<Context, T>;
+  private _cache: WeakMap<Context, ValueOrPromise<T>>;
   private _getValue?: ValueFactory<T>;
 
   /**
@@ -359,18 +359,16 @@ export class Binding<T = BoundValue> extends EventEmitter {
     result: ValueOrPromise<T>,
   ): ValueOrPromise<T> {
     // Initialize the cache as a weakmap keyed by context
-    if (!this._cache) this._cache = new WeakMap<Context, T>();
-    return transformValueOrPromise(result, val => {
-      if (this.scope === BindingScope.SINGLETON) {
-        // Cache the value
-        this._cache.set(ctx.getOwnerContext(this.key)!, val);
-      } else if (this.scope === BindingScope.CONTEXT) {
-        // Cache the value at the current context
-        this._cache.set(ctx, val);
-      }
-      // Do not cache for `TRANSIENT`
-      return val;
-    });
+    if (!this._cache) this._cache = new WeakMap<Context, ValueOrPromise<T>>();
+    if (this.scope === BindingScope.SINGLETON) {
+      // Cache the value
+      this._cache.set(ctx.getOwnerContext(this.key)!, result);
+    } else if (this.scope === BindingScope.CONTEXT) {
+      // Cache the value at the current context
+      this._cache.set(ctx, result);
+    }
+    // Do not cache for `TRANSIENT`
+    return result;
   }
 
   /**