feat(context): introduce context events and observers for bind/unbind

[raymondfeng]

Spin-off from #2122

• Add support for context to emit bind and unbind events
• Allow observers to register with the context chain and respond to come and go of bindings of interest

Followed-up PRs:

#2122 (depends on #2291)

• Introduce ContextView to dynamically resolve values of matching bindings
• Introduce @inject.view to support dependency injection of multiple bound values
• Extend @Inject and @inject.getter to support multiple bound values

#2249

• Add an example package to illustrate how to implement extension point/extension pattern using LoopBack 4’s IoC and DI container

#2259

• Propose new APIs for Context to configure bindings
• Add @inject.config to simplify injection of configuration

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

[bajtos]

Thank you @raymondfeng for extracting this smaller patch, is so much easier to review! Thanks to the smaller size, I was able to spot few areas that deserve further discussion, PTAL below.

@jannyHou @hacksparrow I'd like you to thoroughly review this pull request too, especially the high-level architecture.

[bajtos]

My first and my last comments are related, the order in which notifications are processed depends on the way how we schedule queue-processing callbacks.

Besides the impact on the behavior observed by Context consumers, we should also consider performance implications. Scheduling a new process.nextTick callback for every new binding is not very efficient.

[hacksparrow]

The interface looks good. However, as pointed out by Miroslav, notification processing needs some more work, maybe resulting in additions to to the API. Let's the keep the discussion going.

[raymondfeng]

This isn't going to work well. The promise returned by the async function is ignored by process.nextTick. When an error happens, a warning "unhandled promise rejection" is triggered. In the future versions of Node, this can crash the app the same way as "uncaught error" does already.

We have the following implementation:

protected notifyListeners(
    event: ContextEventType,
    binding: Readonly<Binding<unknown>>,
  ) {
    // Notify listeners in the next tick
    process.nextTick(async () => {
      for (const listener of this.listeners) {
        if (!listener.filter || listener.filter(binding)) {
          try {
            await listener.listen(event, binding);
          } catch (err) {
            debug('Error thrown by a listener is ignored', err, event, binding);
            // Ignore the error
          }
        }
      }
    });
  }

Even though the function for nextTick is async, errors thrown from listeners are caught and logged. There is no chance for unhandled errors to happen. IMO, it's a simple fire-and-forget implementation.

[raymondfeng]

@bajtos @hacksparrow Thank you for the feedback. I now use https://github.com/jessetane/queue to handle event notifications. PTAL.

[hacksparrow]

Good update @raymondfeng. Some more feedback.

The filter method of ContextEventListener should also be optionally async and tested. Eg usecase: I want to check the existence of a file asynchronously and listen for binding events.

Test cases should be also added to document how errors are handled in sync and sync filter methods.

We should be able to tell where in the context chain, an error originated from.

[bajtos]

Let's use Bluebird's convention of adding Async postfix to promise-enabled functions.

Suggested change

	const setImmediatePromise = promisify(setImmediate);
	const setImmediateAsync = promisify(setImmediate);

[raymondfeng]

I copied it from https://nodejs.org/api/timers.html#timers_setimmediate_callback_args but I'm open to any convention.

[bajtos]

Even though the function for nextTick is async, errors thrown from listeners are caught and logged. There is no chance for unhandled errors to happen. IMO, it's a simple fire-and-forget implementation.

FWIW, I see at least two potential places that can trigger unhandled error:

for (const listener of this.listeners) {
// ^^^ this.listeners may not be an iterate-able object

  if (!listener.filter || listener.filter(binding)) {
  // ^^^ listener.filter may be something else than a function

[bajtos]

I now use https://github.com/jessetane/queue to handle event notifications.

What were the criteria on which you chose this particular module? I did a quick search on npm (https://www.npmjs.com/search?q=queue) and p-queue seems to be more popular (p-queue: 283k downloads/month vs queue: 57k downloads/months) and also has more ergonomic API based on promises, not events.

class Context {
  // ...
  async waitUntilEventsProcessed() {
    await this.eventQueue.onIdle();
  }
}

The downside I see is that p-queue does forward errors from individual tasks to the queue instance :(

---

Setting the choice of a queue library aside. It's not clear to me what are the benefits of adding a queue library, when we are still calling process.nextTick for every binding added? I'd prefer to see a solution where we call process.nextTick only once for all bindings created from a single tick of event loop.

[bajtos]

Few more comments specific to individual lines of code.

[bajtos]

The filter method of ContextEventListener should also be optionally async and tested. Eg usecase: I want to check the existence of a file asynchronously and listen for binding events.

Uh oh, I am not a fan of making the filter async. I am concerned it can complicate the code too much. Remember, we are not implementing a generic observer/listener API here, but a specific feature to allow LB4 components to be notified when particular bindings are added or removed.

A typical use case: @loopback/rest wants to be notified when a controller or a handler route was added or removed, so that it can update the routing table. For these use cases, a synchronous filter is good enough.

Code wishing to filter bindings asynchronously should implement filtering inside listen function.

{
  filter: b => true,
  listen: async (event, binding) {
    if (!await shouldProcess(binding)) return;
    // handle the event
  }
}

FWIW, I was proposing to remove filter property entirely in our earlier discussions.

[bajtos]

@raymondfeng please take a look at the following use-case/scenario, I believe it's not covered by your patch yet:

As an app developer, I want to register my error listener on the application context and receive errors from child contexts too, e.g. from per-request contexts.

[hacksparrow]

FWIW, I was proposing to remove filter property entirely in our earlier discussions.

Hmm, I like this approach. @raymondfeng what do you say?

[raymondfeng]

FWIW, I was proposing to remove filter property entirely in our earlier discussions.

Hmm, I like this approach. @raymondfeng what do you say?

The filter has been made optional and default to all bindings if not present. I prefer to keep it to simplify my primary use case of this feature - implementing extension point/extension pattern by receiving notifications of matching extension bindings.

[raymondfeng]

The filter method of ContextEventListener should also be optionally async and tested. Eg usecase: I want to check the existence of a file asynchronously and listen for binding events.

It is intentional to have filter functions to be sync. The purpose is to match bindings of interest for notifications. I don't see a need to perform any async operations.

[raymondfeng]

What were the criteria on which you chose this particular module?

I only found queue but I'm open to other modules. As you pointed out, p-queue does not emit error events at queue level and it makes error handling very hard.

Setting the choice of a queue library aside. It's not clear to me what are the benefits of adding a queue library, when we are still calling process.nextTick for every binding added? I'd prefer to see a solution where we call process.nextTick only once for all bindings created from a single tick of event loop.

I'm adding the queue based on your comments (we should maintain a queue of pending notifications). Considering most bindings are added at application boot/start, it should not matter too much whether we batch the notifications or not.

[raymondfeng]

The key challenge is two-fold:

1. To decide when to notify listeners of binding events because our Context APIs are synchronous and fluent, such as ctx.bind('foo').to('bar').tag('t1');. A listener needs to know the fully populated binding upon notification.

2. How to receive feedback (errors) from context event listeners which are notified asynchronously.

The proposed design in this PR is to use a background task queue to perform notifications in ticks after the binding is added/removed from the context via sync apis.

[raymondfeng]

As an app developer, I want to register my error listener on the application context and receive errors from child contexts too, e.g. from per-request contexts.

I wonder if we can expose Context.prototype.eventQueue as eventEmitter so that error handlers can be registered. For example:

ctx.eventEmitter.on('error', ...);

The other option is to have Context extend EventEmitter.

[raymondfeng]

Considering the difficulty of implementing good error handling, should we perhaps mandate that event observers must not throw, and report any unhandled errors via console.warn, or even crash the process the hard way?

+💯.

[raymondfeng]

I noticed you no longer need to call process.nextTick. How are the notifications deferred now? Is this handled by the async iterator provided by p-event? Do we use Promise micro-queue instead of Node.js event-loop-ticks now?

Now it happens within Promise microtask queue as implemented by p-event.

[raymondfeng]

@bajtos PTAL

[bajtos]

In your implementation, you are trading off performance of event notification for performance of adding a new observer:

• when a new observer is added, we preserve the Set instance in this.observers
• when a new event is fired, a new Set instance is created

I expect that events will be triggered more often than new observers are added, therefore we should do a different trade-off:

• when a new observer is added, a new Set instance is created, it replaces the value stored in this.observers
• when a new event is fired, we take a reference to the Set instance in this.observers because this instance is considered as immutable

Feel free to fix this in a follow-up pull request if you prefer.

[raymondfeng]

A new notification event is only fired when there are observers. I'll defer this micro-optimization.

[bajtos]

A general observation: the file context.ts is almost 700 lines long. I feel that's too much, I read it as a sign that our Context class is doing too much. It would be great to find a way how to move some parts of the functionality into smaller blocks that can live in a different file, and then use composition to leverage those building blocks in Context.

Let's leave such refactoring out of scope of this pull request though, we should focus on getting the initial implementation finished and merged now.

[raymondfeng]

@bajtos PTAL

[bajtos]

Looks mostly good.

I'd like to @jannyHou and @hacksparrow to review these changes before approving the patch.

[jannyHou]

I am able to understand the usage of subscribing multiple observers for a given context. And how the sync/async observe function are handled differently.
It would cost me more time to review the very detail of mechanism of notification, and to be realistic, I would like to see this PR merged to unblock the authentication extension story. And we always have a chance to improve.

I have a design related question: why do we introduce a Subscription interface to handle the unsubscribe action? I also added a short comment in the corresponding test case. People can unsubscribe an observer by calling either ctx.unsubscribe() or subscription.unsubscribe(), what would be the difference?

[jannyHou]

Why having two ways to unsubscribe an observer?

[raymondfeng]

For subscription.subscribe(), we don't need to remember the observer. It follows https://github.com/tc39/proposal-observable

[jannyHou]

an extra |?

[raymondfeng]

That's added by prettier.

[bajtos]

LGTM.

[bajtos]

I think this debug statement will be confusing to our users - it prints the error only but does not give any context where is the error coming from (a context-event listener).

I am proposing to remove this _debug statement and include the error in the debug statements on lines 215 and 220 below.