Fix the issue rawStream is paused in newer versions of Node

[UyumazHakan]

This fixes the issue rawStream get paused and cannot pass check in https://github.com/awolden/brakes/blob/main/lib/globalStats.js#L54 after processing the first snapshot in the newer versions of Node. It is the issue mentioned in #120.

[awolden]

Thanks for PR'ing this fix @UyumazHakan. I will try to get it merged and published today.

[awolden]

Hi @UyumazHakan I was unable to verify this fix worked in versions of node >12.6.0. I was able to fix the issue by removing this pause check: https://github.com/awolden/brakes/compare/remove-pause-check?expand=1. It seems like the behavior of this functionality has changed in 12.6.0 in a way that isn't documented.

Before I go any further tho, can you tell me how you were able to reproduce and verify this change worked, so I can make sure I am not missing something?

[UyumazHakan]

Hi @awolden,

I used the example in #120. I tried with versions 14.15, 12.20 and 10.23. The stream was flowing and I was able to get logs of hystrix continuously (It was one hystrix before this for 14.15 and 12.20.).

I have also considered to delete the check. However, I thought it might harm library when it is running on older versions of Node, since it was a fix for memory leak in #60. I didn't try any version which was available during this PR though.

[awolden]

Ah interesting, that matches what I tried to do unsuccessfully. Do you mind sharing more details such as the code you are using to connect the hystrix stream and your local/deployed running environment?

Removing the pause check does introduce the possibility of a memory leak but there are likely other ways to eliminate that issue.

[UyumazHakan]

I tried with the following code. Before fix, it was only printing one stats data and stops there. After fix, it was able to print stats continuously. I agree that deleting pause check would be better approach and tried it. However, stream lib changed a lot since 4.2.0 and I couldn't come up with a clean way that would be compliant all versions.

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)

[awolden]

Interesting. I have been testing by piping it into an express response for SSE: https://github.com/awolden/brakes/blob/main/examples/hystrix-example.js And it does not appear to fix the issue. However, I found that by adding an additional:

    if (!this._rawStream.isPaused() || this._rawStream.readableFlowing) {
      this._rawStream.push(JSON.stringify(stats));
    }

to these PR change I was able to get it working. I will do some more experimenting before merging this PR and making this addtional change.

[UyumazHakan]

According to documentation, this._rawStream.readableFlowing should be always true in that line. Because, rawStream is piped in constructor and there is no unpipe method called on the rawStream. I think that it will have the same effect with deleting isPaused call.

readableFlowing property is added in v9.4.0. But, I think it will be no problem since there is no issue in older versions and undefined will not break anything here.

[awolden]

Backpressure from the this._hysterixStream will cause readableFlowing to register as false.

Calling readable.pause(), readable.unpipe(), or receiving backpressure will cause the readable.readableFlowing to be set as false.

In my experiments on all node versions of node this seems to create the behavior desired in this module, while guaranteeing no run away memory leak due to back pressure.

I think these 2 changes together are good to go 👍

[UyumazHakan]

Ah, okay! Thank you @awolden . I will be waiting for the release 🙂