Service gets registered multiple times and is corrupting action ctx.params

hhchristian commented 3 years ago

Prerequisites

Please answer the following questions for yourself before submitting an issue.

[x] I am running the latest version
[x] I checked the documentation and found no answer
[x] I checked to make sure that this issue has not already been filed
[x] I'm reporting the issue to the correct repository

Current Behavior

When Service.parseServiceSchema() is called by used code, the service gets registered twice, but the second instance of the service is corrupted and passes incorrect arguments to action/event implementations of the service. This is causing issues when using the hot-reload feature (registering services after broker has been started). In this case, middleware methods are also called twice for each service action call.

Expected Behavior

The Service.parseServiceSchema() method shouldn't cause side effects when beeing called by user code. Service schema and service instance should be distinguished by Moleculer methods Broker.createService() and Service.constructor(). When the Moleculer API allows either to be passed to Broker.createService(), a service instance which probably already has actions, events and _serviceSpecification fields must not be handled similar to a service schema. I'd also appreciate sanity checks at a few significant code locations in Moleculer to detect incorrect calls or wrong arguments and prevent Moleculer from operating incorrectly in such situations.

Failure Information

This issue was observed when using Moleculer with TypeScript and creating services on demand when the Moleculer broker already has been started. The effect of undefined action parameters causing errors in user code is a side effect of the actual issue.

Steps to Reproduce

This issue can be reproduced by modifying the GreeterService TypeScript test script /master/test/typescript/hello-world/index.ts a little bit: Move the broker.loadService(...) call in the async function after await broker.start(); and add a small delay (to internally allow Broker._restartService() to complete), so the first five statements of the async function() { are:

    await broker.start();
    broker.loadService(path.join(__dirname, "greeter.service.ts"));
    await new Promise((res) => setTimeout(res, 50));
    await broker.call("greeter.hello");
    const res = await broker.call("greeter.welcome", { name: "Typescript" });

This modification triggers the issue and is causing the test to fail because the greeter.welcome action implementation in greeter.service.ts and its before hook with method argument ctx: Context<GreeterWelcomeParams> is called with ctx.params = ctx incorrect parameters. This causes the welcome action before hook ctx.params.name = ctx.params.name.toUpperCase(); to raise TypeError: Cannot read property 'toUpperCase' of undefined (because ctx has no name field).

Context

This strange behavior is caused by method Service.parseServiceSchema(schema) being called twice for the/each service:

first time directly in constructor of class GreeterService (as part of the test / example source code in greeter.service.ts),
second time in context of Broker.loadService() ➜ Broker.createService() ➜ new this.ServiceFactory(this, s) ➜ Service.constructor() ➜ Service.parseServiceSchema().

Note:

The class Service has a constructor(broker, schema) with two arguments, but in constructor of class GreeterService (extends Service) it's called as super(broker); with only one argument. When the constructor of class GreeterService would call its base class constructor as super(broker, schema);, method Service.parseServiceSchema(schema) would even be called three times.

Effect of `ctx.params` being `undefined`

The implementation of Service.parseServiceSchema(schema) sets this.actions[name] = (params, opts) => { ... and this.events[innerEvent.name] = (params, opts) => {.

In first call, this initializes the actions and events fields of the service instance with lambda functions having two arguments (params, opts) and calling wrappedHandler(ctx).
But in second call, the service instance is processed as if it were a service schema, again wrapping the actions and events fields (wogether with all middleware calls). This seems to work when having a brief look at the GreeterService test / example, but is causing confusion and strange effects like the ctx argument of service action implementation to have ctx.params set to ctx in some cases - depending on the time/order of service registration.

Dependency on hot-(re)loaded services

The different behavior when registering according services before or after the Moleculer broker has been started seems to be related with the following observation: When the service is registered before the broker is started, the additional service and its endpoints isn't used and only seem to affect metrices. But if a service is registered when the broker already has been started, the second parseServiceSchema() call "wins" and the incorrectly wrapped Service instance is registered by the following function call chain:

➜ Broker.loadService / Broker.createService() ➜ Broker._restartService() ➜ Service._start() ➜ this.broker.registerLocalService(this._serviceSpecification)

Sanity checks:

At the end of Service.parseServiceSchema() method Service._init() and this.broker.addLocalService(this) is called, causing Broker.services[] to contain multiple entries for the same service. This would be a good location for a simple and cheap sanity check which would have detected the issue much earlier than the TypeError: Cannot read property 'toUpperCase' of undefined in external code.

Advantage of strict code and precise expectations

Because both service schema and service instance have actions, and the code in Service._createAction(actionDef, name) can handle the actionDef argument both as object and as function (by wrapping the function into an object), the service instance seems to also work when being used as service schema. Its probably better to be more strict and don't allow all function -or- object -or- array by fixing the argument variation internally. This also would have allowed the issue to be detected earlier. Also being strict instead of allowing variations keeps the code more concise, clear to developers and helps JIT compilers to generate faster code with less variations.

Possible workarounds

Until a fix for this issue is available, it can be workarounded by:

Fixing action handler arguments by patching ContextFactory.create() from "outside" of Moleculer:

// WORKAROUND FOR MOLECULER issue (action handler arguments: ctx.params = ctx)
const origCtxFacCrt = broker.ContextFactory.create;
(broker as any).ContextFactory.create = (brkr, endpt, params, opts) =>
  ((params && params.broker && params.meta && params.params && params.service)
    ? params : origCtxFacCrt(brkr, endpt, params, opts));

Note that the duplicate service registration may still cause problems in this case.

Don't call this.parseServiceSchema in constructor of service classes (like GreeterService). Instead, assign according members of this service instance:
```
this.name = '...';
this.hooks = <ServiceHooks>{ ... };
this.actions = <ServiceActionsSchema>{ ... } as ServiceActions;
```

icebob commented 3 years ago

I tried your reproduce steps, but no issue.

hhchristian commented 3 years ago

I believe that I observed the issue both with broker.loadService and broker.createService, but now it's only reproducible with broker.createService. Maybe there's another precondition I haven't recognized, I'm sorry for this confusion. The following code (modified the GreeterService index.ts TypeScript test script) should trigger the issue:

import * as path from "path";
import { ServiceBroker } from "../../../";
import GreeterService from "./greeter.service";

(async function() {
    try {
        const broker = new ServiceBroker({logger: true});
        await broker.start();

        // broker.loadService(path.join(__dirname, "greeter.service.ts"));
        broker.createService(new GreeterService(broker));
        await new Promise((res) => setTimeout(res, 50));
        const res = await broker.call("greeter.welcome", { name: "Typescript" });

        broker.logger.info("Result: ", res);
        if (res != "Welcome, TYPESCRIPT!") throw new Error("Result is mismatch!");
        await broker.stop();
    } catch(err) {
        console.log(err);
        process.exit(1);
    }
})();

It's using ordinary TypeScript import GreeterService from "./greeter.service"; module loading mechanism and new GreeterService(broker) to construct service instance. We are using Webpack to create a bundle which contains the service modules and therefore cannot dynamically load these modules by their file name or use broker.loadService(fileName), so afaik we have to use broker.createService(serviceInstance).

hhchristian commented 3 years ago

Note: During my experiments I sometimes wasn't able to access members of service schema, service instance, service class (TypeScript) or service base (ServiceFactory). To access information/members/functions of these objects, it's probably necessary to connect JS prototypes - as it's already done in broker.normalizeSchemaConstructor() for service schema and ServiceFactory. In the "TypeScript use case" there's the service class and the object passed to parseServiceSchema() - both of them seem to be relevant. Maybe there's another more elegant solution, but no of my other approaches provides access to all information sources.

hhchristian commented 3 years ago

Note: To workaround the issue in our code, we are currently the following code in service classes:

  constructor(public broker: ServiceBroker) {
    super(broker);
    const self = this;
    // eslint-disable-next-line no-proto
    this.started = function started() { self.__proto__.__proto__ = this; };

    this.actions = <ServiceActionsSchema>{
      myActionFoo: (ctx: Context<ActionFooParams, any>):
        Promise<ActionFooResponse> => this.myActionFooHandler(ctx),
      ...
    } as ServiceActions;

    this.events = <ServiceEvents> {
      myEventBar: {
        handler: (ctx: Context<EventBar, any>): void => {
          this.myEventBarHandler(ctx);
        },
      },
      ...
    };
  }

This class hierarchy manipulation by changing __proto__ of TypeScript service class from Moleculer ServiceFactory/base class to Moleculer service class provides access to members of all involved classes/objects.

icebob commented 1 year ago

I'm closing this issue because we don't know, how we can solve it inside Moleculer. If you have an idea or solution please reopen or open a PR.

moleculerjs / moleculer