Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove close listener before next startStreamProcess loop #1038

Closed
wants to merge 2 commits into from

Conversation

patrykwegrzyn
Copy link

This cause memory leak during fast publishing when leveldb store is used
VM38 warning.js:27 (node:2090114) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [MqttClient]. Use emitter.setMaxListeners() to increase limit

This cause memory leak during fast publishing when leveldb store is used
```VM38 warning.js:27 (node:2090114) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 close listeners added to [MqttClient]. Use emitter.setMaxListeners() to increase limit```
@codecov
Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (98e9a46) 93.63% compared to head (f9a8c28) 93.63%.

❗ Current head f9a8c28 differs from pull request most recent head ff9f040. Consider uploading reports for the commit ff9f040 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1038   +/-   ##
=======================================
  Coverage   93.63%   93.63%           
=======================================
  Files           8        8           
  Lines         942      943    +1     
  Branches      249      249           
=======================================
+ Hits          882      883    +1     
  Misses         60       60           
Impacted Files Coverage Δ
lib/client.js 96.65% <100.00%> (+<0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rtuin
Copy link

rtuin commented Jul 23, 2020

I can't tell much about if this is implemented correctly, but I know it solves the same problem for me.

@YoDaMa
Copy link
Contributor

YoDaMa commented Jul 23, 2020

@rtuin can you explain the problem in a bit more detail? There is a memory leak when publishing in general or just when using leveldb store?

@YoDaMa YoDaMa closed this Jul 23, 2020
@YoDaMa YoDaMa reopened this Jul 23, 2020
@rtuin
Copy link

rtuin commented Jul 27, 2020

@YoDaMa I ran into this issue while writing a simple load generator to run some tests.

Let me elaborate with a code sample. Hope this helps!

var mqtt = require('async-mqtt');
const fs = require('fs');

const runAsync = async () => {
    var client = await mqtt.connect('mqtts://my-host.domain', {
        key: fs.readFileSync('client.key'),
        cert: fs.readFileSync('client.crt'),
        ca: [ fs.readFileSync('ca.crt') ]
    });
     
    let count = 0;
    const publishes = [];
    while (count++ < 100000) {
        publishes.push(() => {
            return client.publish('my-topic', 'Hello mqtt' + count, {qos: 2});
        });
    }

    await Promise.allConcurrent(1000)(publishes).then((x) => {
        console.log(x);
    });

    return await client.end();
};

runAsync();

@robertsLando
Copy link
Member

@patrykwegrzyn Can you sync with master so I can merge this?

@robertsLando
Copy link
Member

If you wish to continue with this PR please remember to change destination branch from master to main 🙏🏼

@robertsLando robertsLando changed the title Remove close listener before next startStreamProcess loop fix: remove close listener before next startStreamProcess loop Jun 30, 2023
@robertsLando
Copy link
Member

Supersided by #1759

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants