feat(context): leave local bindings and parent unchanged during close

See #2541

Author: raymondfeng

packages/context/src/__tests__/acceptance/interceptor.acceptance.ts

@@ -171,6 +171,46 @@ describe('Interceptor', () => {
     expect(events).to.eql(['logError: before-greet', 'logError: error-greet']);
   });
 
+  it('closes invocation context after invocation', async () => {
+    const testInterceptor: Interceptor = async (invocationCtx, next) => {
+      // Add observers to the invocation context, which in turn adds listeners
+      // to its parent - `ctx`
+      invocationCtx.subscribe(() => {});
+      return await next();
+    };
+
+    class MyController {
+      @intercept(testInterceptor)
+      async greet(name: string) {
+        return `Hello, ${name}`;
+      }
+    }
+
+    // No listeners yet
+    expect(ctx.listenerCount('bind')).to.eql(0);
+    const controller = new MyController();
+
+    // Run the invocation 5 times
+    for (let i = 0; i < 5; i++) {
+      const count = ctx.listenerCount('bind');
+      const invocationPromise = invokeMethodWithInterceptors(
+        ctx,
+        controller,
+        'greet',
+        ['John'],
+      );
+      // New listeners are added to `ctx`
+      expect(ctx.listenerCount('bind')).to.be.greaterThan(count);
+
+      // Wait until the invocation finishes
+      await invocationPromise;
+    }
+
+    // Listeners added by invocation context are gone now
+    // There is one left for ctx.observers
+    expect(ctx.listenerCount('bind')).to.eql(1);
+  });
+
   it('invokes static interceptors', async () => {
     class MyController {
       // Apply `log` to a static method

packages/context/src/__tests__/unit/context.unit.ts

@@ -11,6 +11,7 @@ import {
   BindingScope,
   BindingType,
   Context,
+  ContextEventObserver,
   isPromiseLike,
 } from '../..';
 
@@ -19,13 +20,17 @@ import {
  * for assertions
  */
 class TestContext extends Context {
+  observers: Set<ContextEventObserver> | undefined;
   get parent() {
     return this._parent;
   }
   get bindingMap() {
     const map = new Map(this.registry);
     return map;
   }
+  get parentEventListeners() {
+    return this._parentEventListeners;
+  }
 }
 
 describe('Context constructor', () => {
@@ -725,18 +730,48 @@ describe('Context', () => {
   });
 
   describe('close()', () => {
-    it('clears all bindings', () => {
-      ctx.bind('foo').to('foo-value');
-      expect(ctx.bindingMap.size).to.eql(1);
-      ctx.close();
-      expect(ctx.bindingMap.size).to.eql(0);
+    it('clears all observers', () => {
+      const childCtx = new TestContext(ctx);
+      childCtx.subscribe(() => {});
+      expect(childCtx.observers!.size).to.eql(1);
+      childCtx.close();
+      expect(childCtx.observers).to.be.undefined();
     });
 
-    it('dereferences parent', () => {
+    it('removes listeners from parent context', () => {
       const childCtx = new TestContext(ctx);
-      expect(childCtx.parent).to.equal(ctx);
+      childCtx.subscribe(() => {});
+      // Now we have one observer
+      expect(childCtx.observers!.size).to.eql(1);
+      // Two listeners are also added to the parent context
+      const parentEventListeners = childCtx.parentEventListeners!;
+      expect(parentEventListeners.size).to.eql(2);
+
+      // The map contains listeners added to the parent context
+      // Take a snapshot into `copy`
+      const copy = new Map(parentEventListeners);
+      for (const [key, val] of copy.entries()) {
+        expect(val).to.be.a.Function();
+        expect(ctx.listeners(key)).to.containEql(val);
+      }
+
+      // Now clear subscriptions
+      childCtx.close();
+
+      // observers are gone
+      expect(childCtx.observers).to.be.undefined();
+      // listeners are removed from parent context
+      for (const [key, val] of copy.entries()) {
+        expect(ctx.listeners(key)).to.not.containEql(val);
+      }
+    });
+
+    it('keeps parent and bindings', () => {
+      const childCtx = new TestContext(ctx);
+      childCtx.bind('foo').to('foo-value');
       childCtx.close();
-      expect(childCtx.parent).to.be.undefined();
+      expect(childCtx.parent).to.equal(ctx);
+      expect(childCtx.contains('foo'));
     });
   });
 

packages/context/src/context.ts

@@ -403,9 +403,10 @@ export class Context extends EventEmitter {
   }
 
   /**
-   * Close the context and release references to other objects in the context
-   * chain.
+   * Close the context: clear observers, stop notifications, and remove event
+   * listeners from its parent context.
    *
+   * @remarks
    * This method MUST be called to avoid memory leaks once a context object is
    * no longer needed and should be recycled. An example is the `RequestContext`,
    * which is created per request.
@@ -426,8 +427,6 @@ export class Context extends EventEmitter {
       }
       this._parentEventListeners = undefined;
     }
-    this.registry.clear();
-    this._parent = undefined;
   }
 
   /**

packages/rest/src/__tests__/integration/request-context.integration.ts

@@ -9,13 +9,13 @@ import {
   createRestAppClient,
   expect,
   givenHttpServerConfig,
-  supertest,
   httpsGetAsync,
+  supertest,
 } from '@loopback/testlab';
 import * as express from 'express';
 import {RequestContext} from '../../request-context';
 import {RestApplication} from '../../rest.application';
-import {RestServerConfig} from '../../rest.server';
+import {RestServer, RestServerConfig} from '../../rest.server';
 import {DefaultSequence} from '../../sequence';
 
 let app: RestApplication;
@@ -116,6 +116,22 @@ describe('RequestContext', () => {
   });
 });
 
+describe('close', () => {
+  it('removes listeners from parent context', async () => {
+    await givenRunningAppWithClient();
+    const server = await app.getServer(RestServer);
+    // Running the request 5 times
+    for (let i = 0; i < 5; i++) {
+      await client
+        .get('/products')
+        .set('x-forwarded-proto', 'https')
+        .expect(200);
+    }
+    expect(observedCtx.contains('req.originalUrl'));
+    expect(server.listenerCount('bind')).to.eql(1);
+  });
+});
+
 function setup() {
   (app as unknown) = undefined;
   (client as unknown) = undefined;
@@ -141,5 +157,9 @@ function contextObservingHandler(
   _sequence: DefaultSequence,
 ) {
   observedCtx = ctx;
+  // Add a subscriber to verify `close()`
+  ctx.subscribe(() => {});
+  // Add a binding to the request context
+  ctx.bind('req.originalUrl').to(ctx.request.originalUrl);
   ctx.response.end('ok');
 }

packages/rest/src/request-context.ts

@@ -95,6 +95,8 @@ export class RequestContext extends Context implements HandlerContext {
     super(parent, name);
     this._setupBindings(request, response);
     onFinished(this.response, () => {
+      // Close the request context when the http response is finished so that
+      // it can be recycled by GC
       this.close();
     });
   }