New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 12, 2019

Conversation

Projects
None yet
5 participants
@raymondfeng
Copy link
Member

raymondfeng commented Jan 25, 2019

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

@raymondfeng raymondfeng requested a review from bajtos as a code owner Jan 25, 2019

@raymondfeng raymondfeng force-pushed the add-context-listener branch from c02bb89 to 2baebe8 Jan 25, 2019

@bajtos
Copy link
Member

bajtos left a comment

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.

Show resolved Hide resolved packages/context/src/context.ts Outdated
Show resolved Hide resolved packages/context/src/context.ts Outdated
Show resolved Hide resolved packages/context/test/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/test/unit/context.unit.ts Outdated

@bajtos bajtos requested review from jannyHou and hacksparrow Jan 28, 2019

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 28, 2019

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

This comment has been minimized.

Copy link
Member

hacksparrow commented Jan 28, 2019

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 comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 28, 2019

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 raymondfeng force-pushed the add-context-listener branch from 2baebe8 to 97316a6 Jan 28, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 28, 2019

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

@raymondfeng raymondfeng force-pushed the add-context-listener branch 4 times, most recently from 7ebd4b5 to 43f21c0 Jan 28, 2019

@hacksparrow

This comment has been minimized.

Copy link
Member

hacksparrow commented Jan 29, 2019

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.

} from '../..';

import {promisify} from 'util';
const setImmediatePromise = promisify(setImmediate);

This comment has been minimized.

@bajtos

bajtos Jan 29, 2019

Member

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

Suggested change Beta
const setImmediatePromise = promisify(setImmediate);
const setImmediateAsync = promisify(setImmediate);

This comment has been minimized.

@raymondfeng

raymondfeng Jan 29, 2019

Author Member

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

Show resolved Hide resolved packages/context/test/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/test/unit/context.unit.ts Outdated
Show resolved Hide resolved packages/context/src/context.ts Outdated
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 29, 2019

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

This comment has been minimized.

Copy link
Member

bajtos commented Jan 29, 2019

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
Copy link
Member

bajtos left a comment

Few more comments specific to individual lines of code.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Jan 29, 2019

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

This comment has been minimized.

Copy link
Member

bajtos commented Jan 29, 2019

@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

This comment has been minimized.

Copy link
Member

hacksparrow commented Jan 29, 2019

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

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

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 29, 2019

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

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 29, 2019

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

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 29, 2019

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

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 29, 2019

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 raymondfeng force-pushed the add-context-listener branch 2 times, most recently from f6c793e to 2b07610 Jan 29, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Jan 29, 2019

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 raymondfeng force-pushed the add-context-listener branch from 41c45b4 to fd3464d Feb 1, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 1, 2019

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 raymondfeng force-pushed the add-context-listener branch 3 times, most recently from aa9c9fc to 1b41d1d Feb 1, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 4, 2019

@bajtos PTAL

@raymondfeng raymondfeng force-pushed the add-context-listener branch from 1b41d1d to 9309d59 Feb 4, 2019

Show resolved Hide resolved packages/context/src/context.ts
Show resolved Hide resolved packages/context/src/context.ts Outdated
// current context observers.
this.emit(notificationEvent, {
binding,
observers: new Set(this.observers),

This comment has been minimized.

@bajtos

bajtos Feb 5, 2019

Member

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.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 5, 2019

Author Member

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

Show resolved Hide resolved packages/context/src/context.ts Outdated
Show resolved Hide resolved packages/context/src/context.ts
Show resolved Hide resolved packages/context/test/unit/context-observer.unit.ts Outdated
Show resolved Hide resolved docs/site/Context.md Outdated
Show resolved Hide resolved docs/site/Context.md Outdated
Show resolved Hide resolved docs/site/Context.md Outdated
Show resolved Hide resolved packages/context/src/context.ts Outdated
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Feb 5, 2019

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 raymondfeng force-pushed the add-context-listener branch from 9309d59 to ab00a5e Feb 5, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 5, 2019

@bajtos PTAL

@raymondfeng raymondfeng force-pushed the add-context-listener branch 2 times, most recently from 9425ee6 to 9cb8a4f Feb 5, 2019

@raymondfeng raymondfeng changed the title feat(context): introduce context listener for bind/unbind events feat(context): introduce context events and observers for bind/unbind Feb 5, 2019

@bajtos
Copy link
Member

bajtos left a comment

Looks mostly good.

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

Show resolved Hide resolved docs/site/Context.md
Show resolved Hide resolved packages/context/src/__tests__/unit/context-observer.unit.ts Outdated
Show resolved Hide resolved packages/context/src/context.ts Outdated
Show resolved Hide resolved packages/context/src/context.ts Outdated

@raymondfeng raymondfeng force-pushed the add-context-listener branch 3 times, most recently from ffda885 to fb6c24a Feb 7, 2019

@jannyHou
Copy link
Contributor

jannyHou left a comment

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?

it('allows subscription.unsubscribe()', () => {
const subscription = ctx.subscribe(nonMatchingObserver);
expect(ctx.isSubscribed(nonMatchingObserver)).to.true();
subscription.unsubscribe();

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

Why having two ways to unsubscribe an observer?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 12, 2019

Author Member

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

* these listeners when this context is closed.
*/
protected _parentEventListeners:
| Map<

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

an extra |?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 12, 2019

Author Member

That's added by prettier.

@bajtos

bajtos approved these changes Feb 12, 2019

Copy link
Member

bajtos left a comment

LGTM.

* @param err Error
*/
private handleNotificationError(err: unknown) {
this._debug(err);

This comment has been minimized.

@bajtos

bajtos Feb 12, 2019

Member

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.

@raymondfeng raymondfeng force-pushed the add-context-listener branch from fb6c24a to dade36a Feb 12, 2019

@raymondfeng raymondfeng force-pushed the add-context-listener branch from dade36a to d43b75b Feb 12, 2019

@raymondfeng raymondfeng merged commit e5e5fc4 into master Feb 12, 2019

5 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 90.458%
Details
security/snyk - package.json (dhmlau) No manifest changes detected

@raymondfeng raymondfeng deleted the add-context-listener branch Feb 12, 2019

@dhmlau dhmlau added this to the February 2019 milestone milestone Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment