awolden / brakes

Hystrix compliant Node.js Circuit Breaker Library
MIT License
300 stars 35 forks source link

GlobalStatsStream does not track all instances after update to Node v12.16.0 #120

Closed stefanpernpaintner closed 3 years ago

stefanpernpaintner commented 3 years ago

Brakes version: 3.0.1

After having updated to Node v12.16.0 globalStatsStream does not track the instances correctly. I have two brakes instantiated but only the first brake does notify via stream for only one first event. Example:

        const brake1 = new Brakes(promiseCall, {
            name: "MyDefault",
            bucketSpan: 5000,
            bucketNum: 3,
            waitThreshold: 5,
            timeout: 200,
            threshold: 0.5,
            circuitDuration: 30000,
        });

        const brake2 = new Brakes(promiseCall, {
            name: "XXXX",
            bucketSpan: 5000,
            bucketNum: 3,
            waitThreshold: 5,
            timeout: 200,
            threshold: 0.5,
            circuitDuration: 30000,
        });

        Brakes.getGlobalStats().getHystrixStream().on('data', (stats) => {
            console.log('received global stats ->', stats);
        });

With Node v12.15.0 it is working as expected. Alle events are propagated via the stats stream. Any known issues about this?

awolden commented 3 years ago

Hi @stefanpernpaintner thanks for raising this issue. I'm not aware of any issues with newer version of node. Do you see the same issue with other major versions of node such as 14 or 15?

stefanpernpaintner commented 3 years ago

I've tested only with node 14. The issue exists with node 14 too.

stefanpernpaintner commented 3 years ago

Here is a full example with express which shows that only "Brake 1" is considered by the stream if you run it on node > 12.16 :

const express = require('express')
const Brakes = require("brakes");
const app = express()

function promiseCall(foo) {
    return new Promise((resolve, reject) => {
        if (foo) resolve(foo);
        else reject(foo);
    });
}

const brake1 = new Brakes(promiseCall, {
    name: "Brake 1",
    bucketSpan: 5000,
    bucketNum: 3,
    waitThreshold: 5,
    timeout: 200,
    threshold: 0.5,
    circuitDuration: 30000,
});

const brake2 = new Brakes(promiseCall, {
    name: "Brake 2",
    bucketSpan: 5000,
    bucketNum: 3,
    waitThreshold: 5,
    timeout: 200,
    threshold: 0.5,
    circuitDuration: 30000,
});

Brakes.getGlobalStats().getHystrixStream().on('data', (stats) => {
    console.log('received global stats ->', stats);
});

app.get('/', function (req, res) {
    brake1.exec('bar').then(value => {
        console.log(value)
    })
    brake2.exec("test").then(value => {
        console.log(value)
    })
    res.send('Hello World')
})

app.listen(3000)
UyumazHakan commented 3 years ago

The root cause of problem is after first snapshot is processed by _rawStream. The stream gets paused and cannot pass check in main/lib/globalStats.js#L54 . Therefore, it will not able to push anymore snapshots in the stream for newer versions of Node. Please see https://nodejs.org/api/stream.html#stream_compatibility_with_older_node_js_versions . I created a pull request #121 with a fix.

awolden commented 3 years ago

This issue should be fixed in in version 3.1.0. Thank you @UyumazHakan for the collaboration.