debounced localServiceChanged breaking with NATS

alvaroinckot commented 1 year ago

:stop_sign: Do you want to ask a question ?

Please head to the Discord chat

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

Using NATS, if a ServiceBroker with internal services enabled gets stopped during a service change process, it proceeds with the unsubscribeFromBalancedCommands and fails as the NATS client was already flushed in the stop process.

It is very likely to happen during integration tests or when scaling up and down nodes.

Expected Behavior

When the ServiceBroker is stopped, the bounced service change process should be interrupted.

Failure Information

I was able to reproduce it only with NATS, and line of code in the service-broker.js file causing it should be:

this.localServiceChanged = _.debounce(() => origLocalServiceChanged.call(this), 1000);

Steps to Reproduce

Please provide detailed steps for reproducing the issue.

Create multiple service broker connected to NATS
Trigger a service change by creating a new service
Immediately stops the broker

Reproduce code snippet

const moleculer = require("moleculer");

const brokerOne = new moleculer.ServiceBroker({
  transporter: "nats://localhost:4222",
  disableBalancer: true,
  nodeID: "broker-one",
  namespace: "foo",
});
const brokerTwo = new moleculer.ServiceBroker({
  transporter: "nats://localhost:4222",
  disableBalancer: true,
  nodeID: "broker-two",
});
const brokerThree = new moleculer.ServiceBroker({
  transporter: "nats://localhost:4222",
  disableBalancer: true,
  nodeID: "broker-three",
  namespace: "foo",
});

const main = async () => {
  await Promise.all([
    brokerOne.start(),
    brokerTwo.start(),
    brokerThree.start(),
  ]);

  brokerOne.createService({
    name: "foo",
  });

  await Promise.all([
    brokerTwo.createService({
      name: "bar",
    }),
    brokerThree.stop(),
    brokerOne.stop(),
  ]);
};

main();

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

Moleculer version: 0.14.28
NATS lib verison: 2.12.1
NodeJS version: 16.15.1
Operating System: macOS Monterey (12.6)

Failure Logs

<redacted>/moleculer-localservice-racing-condition/node_modules/moleculer/src/transporters/nats.js:314
                        return this.client.flush();
                                           ^

TypeError: Cannot read properties of null (reading 'flush')
    at NatsTransporter.unsubscribeFromBalancedCommands (<redacted>/moleculer-localservice-racing-condition/node_modules/moleculer/src/transporters/nats.js:314:23)
    at NatsTransporter.makeBalancedSubscriptions (<redacted>/moleculer-localservice-racing-condition/node_modules/moleculer/src/transporters/base.js:259:15)
    at LocalDiscoverer.sendLocalNodeInfo (<redacted>/moleculer-localservice-racing-condition/node_modules/moleculer/src/registry/discoverers/local.js:67:23)
    at ServiceBroker.localServiceChanged (<redacted>/moleculer-localservice-racing-condition/node_modules/moleculer/src/service-broker.js:956:28)
    at ServiceBroker.<anonymous> (<redacted>/moleculer-localservice-racing-condition/node_modules/moleculer/src/service-broker.js:306:72)
    at invokeFunc (<redacted>/moleculer-localservice-racing-condition/node_modules/lodash/lodash.js:10401:23)
    at trailingEdge (<redacted>/moleculer-localservice-racing-condition/node_modules/lodash/lodash.js:10450:18)
    at Timeout.timerExpired [as _onTimeout] (<redacted>/moleculer-localservice-racing-condition/node_modules/lodash/lodash.js:10438:18)
    at listOnTimeout (node:internal/timers:559:17)
    at processTimers (node:internal/timers:502:7)

alvaroinckot commented 1 year ago

A fix could be something like?

    /**
     * It's a debounced method to send INFO packets to remote nodes.
     */
    localServiceChanged() {
        if (!this.stopping) { // add this check before sending local node info
            this.registry.discoverer.sendLocalNodeInfo();
        }
    }

EDIT: This snippet does fix the issue, but IDK exactly how to create an unit test case for that. If you guys give me the guidance, I can do it.

HandersonMarinho commented 1 year ago

This would be really helpful 🚀

icebob commented 1 year ago

@alvaroinckot nice catch, please open a PR with your fix. Currently, there is no integration test for balancing logic, only unit tests. So for me, it's enough if you add a unit test for the fix and we can check manually with the repro code. Check this part of tests: https://github.com/moleculerjs/moleculer/blob/master/test/unit/service-broker.spec.js#L1660

alvaroinckot commented 1 year ago

@icebob I just created the PR #1186. Thank you!

moleculerjs / moleculer