Moleculer: Base tracer does not handle circular json objects as tags in action response

[JordanPawlett]

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

flattenTags in the base exporter for tracers throws an error if the response is a circular json structure.

flattenTags is a recursive function. This function will never reach its base-case when a circular json-object is passed in. This could occur accidentally if the brokers tracer is configured to include the response in the tracing tags, and the actions response is naively defined by a third-party library

Method found here

Expected Behavior

flattenTags should not error. It may be sensible to remove the circular-object and continue.

Ultimately the developer should probably handle returning a sensible response structure. However, i believe this is valid as an action response with the current serializer, so a valid trace should also be produced.

Failure Information

RangeError: Maximum call stack size exceeded

Steps to Reproduce

Use a third-party module to handle the response of a particular action, where the response is a circular json-structure

Reproduce code snippet

    config.tracing = {
      tags: {
        action: {
          params: true,
          response: true
        }
      }
    }

    const funcResult = {};
    const objA = { foo: 'bar', parent: funcResult };
    funcResult['objA'] = objA;
    const obj = {
      a: 5,
      b: "John",
      c: {
        d: "d",
        e: {
          f: true,
          g: 100,
        },
        h: null,
        i: undefined
      },
      response: { result: funcResult }
    };

    it('should not throw an error', () => {
      try {
        exporter.flattenTags(obj);
        expect(true).toBe(true);
      } catch (e) {
        fail(e);
      }
    });

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.12
• NodeJS version: 12.15.0
• Operating System: alpine-linux

Failure Logs

(node:27) UnhandledPromiseRejectionWarning: RangeError: Maximum call stack size exceeded
    at Function.keys (<anonymous>)
    at JaegerTraceExporter.flattenTags (/home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:91:17)
    at /home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:96:29
    at Array.reduce (<anonymous>)
    at JaegerTraceExporter.flattenTags (/home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:91:27)
    at /home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:96:29
    at Array.reduce (<anonymous>)
    at JaegerTraceExporter.flattenTags (/home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:91:27)
    at /home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:96:29
    at Array.reduce (<anonymous>)
    at JaegerTraceExporter.flattenTags (/home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:91:27)
    at /home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:96:29
    at Array.reduce (<anonymous>)
    at JaegerTraceExporter.flattenTags (/home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:91:27)
    at /home/service/scheduling-service/node_modules/moleculer/src/tracing/exporters/base.js:96:29
    at Array.reduce (<anonymous>)
(node:27) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 4)

edit:
https://github.com/moleculerjs/moleculer-addons/tree/master/packages/moleculer-bull#readme
createJob from this mixin returns a circular-json structure as a response. Ultimately i think it's either a developer issue, or an issue that should be solved within the flattenTags function though

[intech]

related with https://github.com/moleculerjs/moleculer/issues/777

[JordanPawlett]

Yep definitely related!

I've encountered another related problem with tracer response in the past when sending binary data as a response too, from Service-A -> Service-B. I've had to disable response tracing on this service. Although i suppose at it's root, this is the same as the issues you've just linked.

        ctx.meta.$streamObjectMode = true;
        const stream = new Readable({
          objectMode: true,
          read() { }
        });

        process.nextTick(() => {
          const hits = res.body.hits.hits;

          // producer tells the consumer how many results it's expecting - consuming keeps track of results received.
          stream.push({ data: hits.length, type: 'total' });
          stream.push({ data: res.body, type: 'body' });
          hits.forEach(hit => {
            stream.push({ data: hit, type: 'hit' });
          });
          stream.destroy();
        });
        return stream;

[icebob]

I understand that we would use circular detection codes in every part of Moleculer codes where has object traversing or serialization. But there is a reason why even the built-in JSON serializer has no circular detection because it cuts down the performance significantly.

Here is a benchmark test: (it's still in the benchmark sources)

So a simple circular detection degrades the performance by ~40% which is not negligible.

During the development of Moleculer, I always strive to reach the best performance, this is why the codebase doesn't contain this kind of codes. I know it can protect the programmer from itself, but the framework will be slower and slower.

As described here, you can filter the properties from the response or define custom Function where you can select the properties for the tracing.

[JordanPawlett]

I completely agree it's better to keep the framework lightweight and performant. Especially as it's easy enough for the developer to control their action responses to not include a circular response, or provide a custom tag function!

I did look for a solution before raising the issue, and came to the same conclusion that it wasn't worth the performance hit.

I guess what i'm flagging here is it could take a while for a 'newer' Javascript developer to figure out what the underlying issue is with an obscure error.

(node:27) UnhandledPromiseRejectionWarning: RangeError: Maximum call stack size exceeded
    at Function.keys (<anonymous>)
    at JaegerTraceExporter.flattenTags (/home/service/service/node_modules/moleculer/src/t

Maybe the framework could bubble up a useful error that could suggest what the problem is?

[icebob]

If we start to detect that why it's called too much, it has the same performance degradation.
So I suggest, that we should define a max deep level in the flattenTags. So we only increment a counter which is very lightweight. And if we reach the limit, we throw an error, that "Maybe your tags contain a circular structure....etc". WDYT?

Or not throw just warning?!?

[JordanPawlett]

I was going to suggest adding a cyclic check behind a flag such as process.env.NODE_ENV !== 'production', so it wouldn't be present in production, but would be caught in any integration tests / development.

I think it's sensible idea to increment a counter and throw an error with a sensible message when MAX_DEEP_LEVEL is reached, as Javascript is already throwing a Maximum call stack size exceeded error anyway.

However, the hard part is deciding what is a sensible number... How do you decide how deep to allow a response object to be. This could possibly lead to a breaking change.

Ultimately, the safest bet is probably not to throw an error, just warn, and then cut-off the object at that level, handling successfully and continuing.