Hello,
Looks like Nodemailer is leaking memory. Here's a simple code to reproduce (account details omitted):
const nodemailer = require('nodemailer');
const transport = nodemailer.createTransport({
host: 'email-smtp.us-east-1.amazonaws.com',
port: 465,
auth: {
user: '...',
pass: '...'
}
});
const options = {
from: '...',
to: '[email protected]',
subject: 'test',
html: '<html>hello</html>',
text: 'hello'
};
async function send() {
await transport.sendMail(options);
return send();
}
send();
Just a basic loop that sends one email after another. Leaving it for a couple of minutes results in lots of allocated objects that are not garbage collected, as seen in comparison view:
Both snapshots were taken immediately after manual garbage collection.
Case details:
OS: Ubuntu 18.04.2 LTS
Node: v10.13.0
Nodemailer: 5.1.1
SMTP Server: Amazon SES
kjarmicki
All 30 comments
Update: using Node 8.x or SMTP connection pool seems to help, but not completely - there's still a bit of a leak, albeit an order of magnitude smaller than before.
kjarmicki
on 15 Feb 2019
Just interesting... It looks like you created a recursive loop, so the GC is waiting to exit this loop. IMO
xr0master
on 13 Mar 2019
@xr0master I think this explanation is unlikely, reasons being:
- If that was the case, then switching to Node8 or using connection pool would not help - but it did
- GC can run in the middle of normal loops (source), let alone async recursive calls that do not block the main thread
- Chrome Devtools have an option to manually trigger GC at any time, which I did before taking comparison snapshots
kjarmicki
on 13 Mar 2019
@kjarmicki Maybe you are right.
- I don't know, the stream API has been changed in node v10, maybe it's a reason.
- GC can run any time of course, but it doesn't free memory, if data "in use". Also your code runs in sync mode, so...
I didn't test it, but I think the code below will change your snapshot:
let count = 20;
async function send() {
await transport.sendMail(options);
return --count && send();
}
send();
But again, it is my opinion. I'm not author
xr0master
on 14 Mar 2019
I don't know, the stream API has been changed in node v10, maybe it's a reason.
Ok, that's possible, but using connection pool also alleviates the problem, regardless of Node version
GC can run any time of course, but it doesn't free memory, if data "in use"
And if it can't free the memory when it should that's precisely a memory leak
Also your code runs in sync mode
I don't think there is such thing as "sync mode" for async operations :thinking:
This is only the smallest example I could come up with that reproduces the issue. In my case memory leak was first observed in production system that sends emails on demand rather than in the loop and switching to connection pool helped - which also applies to the small example I provided.
kjarmicki
on 14 Mar 2019
@kjarmicki You are right. I can reproduce it too. Upset, I have this leak too then.
Is your workaround to use the smtp pool? Will check it too, thanks.
xr0master
on 14 Mar 2019
@xr0master yes, using smtp pool helped, but there's still a bit of a leak. It's small enough that we deploy the new version faster than service runs out of memory, but nevertheless, it's there.
Here's how our memory usage pattern changed before and after using smtp pool:
kjarmicki
on 14 Mar 2019
Very interesting. Difference between pooled and non-pooled is that in pooled settings TCP connections are reused to send 100 messages while non-pooled creates new connection for every message.
What happens when you set maxMessages: Infinity in transport settings? Does the memory leak growth get even smaller? This setting makes Nodemailer to use tcp connections as long as possible.
andris9
on 14 Mar 2019
@andris9 exactly, the SMTPConnection creates this leak. If you use the pool for 100 messages, then the memory leak is 100 times slower.
I tried to use the pool, but closed it every time (SMTP settings change every time), the memory leak rate is the same as for a non-pooled connection.
PS. I also checked SES and other custom transports (my own), they do not create a leak. That is, it is definitely a SMTP transport issue.
xr0master
on 14 Mar 2019
@andris9 @kjarmicki
After debugging, I see that the issue is that the code does not detach the events.
If I add removeAllListeners to _destroy method of smtp-connection, then it solves (almost) this issue:
_destroy() {
this.removeAllListeners();
this._socket.removeAllListeners();
if (this._destroyed) {
return;
}
this._destroyed = true;
this.emit('end');
}
Also you need to do it for connection:
let connection = new SMTPConnection(options);
Because the event 'error' is still attached.
connection.once('end', () => {
connection.removeAllListeners();
...
});
After these changes, we can see:
HOWEVER we still have a small memory leak:
xr0master
on 18 Mar 2019
Fixed this small leak memory too:
The issue is setKeepAlive
this._socket.setKeepAlive(true);
So we should to destroy it also.
this._socket.destroy();
All fix will be like:
_destroy() {
this.removeAllListeners();
this._socket.removeAllListeners();
this._socket.destroy();
if (this._destroyed) {
return;
}
this._destroyed = true;
this.emit('end');
}
PS and of course, probably, I did this temporary fix in wrong place.
xr0master
on 18 Mar 2019
Normally you should not have to call removeAllListeners as this only clears the array of methods in socket object – if socket gets garbage collected then the events array gets removed as well.
Would it make a difference if you'd just remove this._socket.setKeepAlive(true); ? It is not even needed as SMTP connections are not long-lived.
andris9
on 18 Mar 2019
Yes. You are right about listeners. I thought it would not work if there is a non-scoped variable in the listener. The issue is that the socket doesn't get garbage collected. I have not found why. I remove the setKeepAlive, it doesn't help.
However, if I destroy it programmatically, calling the method this._socket.destroy(); in _destroy method, then it fixes it.
_destroy() {
if (this._destroyed) {
return;
}
this._socket.destroy();
this._destroyed = true;
this.emit('end');
}
PS Maybe this._socket.setKeepAlive(true); is used for a pool. You know it better than me.
xr0master
on 18 Mar 2019
@andris9 Added PR
xr0master
on 19 Mar 2019
@andris9 Any news? I don’t want to rush anyone, but rebooting the server every day creates an inconvenience. Unfortunately the travis crashes on eslint:all checks.
@kjarmicki Can you confirm that this solution also fixes your problem?
xr0master
on 20 Mar 2019
Calling socket.destroy() might be unsafe, eg. it may throw (for example if the socket was already closed). Additionally, in normal circumstances there should be no need to call destroy() as end() is called anyway and it should close the socket. In your case it seems to me that Nodemailer calls socket.end() which closes half the socket but the other half is not closed for whatever reason and as such the socket is not garbage collected and you end up with this memory leak. No idea what causes it. Something in your network routing or firewall maybe? Virus scanner somewhere messing with email traffic?
So what you really need is not to add an additional socket.destroy() but maybe add a configuration option to use 'destroy' instead of 'end' when closing sockets here
andris9
on 21 Mar 2019
It makes sense...
However, the end() of the socket calls the end event, and then the _onEnd() method calls _destroy(), it looks logical... So I don't think we need to change closeMethod logic. But you are right, it can create a new issue, if this._destroy(); will call again. Also I'm not sure if this socket.end() destroys itself or just creates an event 'end' in Node logic.
I can move this._socket.destroy() code to _onEnd() method. Only socket uses this method, it will safely:
_onEnd() {
this._socket.destroy();
this._destroy();
}
P.S. I tried this code on my locals machines and AWS EC2, I have this happening everywhere.
xr0master
on 21 Mar 2019
From Node.js docs:
- socket.end(): Half-closes the socket. i.e., it sends a FIN packet. It is possible the server will still send some data.
- socket.destroy(): Ensures that no more I/O activity happens on this socket. Only necessary in case of errors.
You could try to run lsof -itcp and look for connections in FIN_WAIT2 state. Normally there should not be any or these should disappear fast.
andris9
on 21 Mar 2019
I see. I will try to create a simplified version of the code where we can reproduce this issue.
xr0master
on 21 Mar 2019
Done: https://github.com/xr0master/nodemailer-memory-leak
I noticed that this happens only if secure is enabled and port 465.
For port 587 is ok
I used yahoo as example, you can change it in host
example in README.md, please set your "to", "from" and "password".
P.S. code in TS but I don't think it can create any troubles.
xr0master
on 21 Mar 2019
Kudos for doing such a deep dive into this issue @xr0master :+1:
kjarmicki
on 21 Mar 2019
@andris9 I think I really-really found this problem :) It's here
In the debugger, I see that the socket already has an onReadableStreamEnd listener for the end event. If we call removeAllListeners('end'), then the stream will never end, and the close event is never called for this socket.
Proof:
xr0master
on 23 Mar 2019
xr0master
on 23 Mar 2019
It seems plausible. Nodemailer is very old and this code has been dragged on from early versions of Node where there used to be a different streams implementation. Removing the handlers might not be safe on some occasions though. What really should happen is to not use removeAllListeners but to use removeListener to remove these handlers that Nodemailer itself has set and not touch other handlers.
andris9
on 23 Mar 2019
You are right, ideally, the use of removeAllListeners is possible only in the destructor, and to use it somewhere else is not recommended. On good, it is worth rewriting SMTP connection module.
If use removeListener, then this requires to rewrite all anonymous callbacks for socket.
P.S. I took that code from _upgradeConnection method.
xr0master
on 23 Mar 2019
@andris9 https://github.com/nodemailer/nodemailer/pull/990
PR using removeListener
xr0master
on 23 Mar 2019
@xr0master thanks for the effort! I merged the PR and released as v6.0.0.
andris9
on 25 Mar 2019
@andris9 Thank you for supporting and reviewing the code.
xr0master
on 25 Mar 2019
Related issues
miranagha
·
8Comments
jonathanong
·
12Comments
renanbastos93
·
11Comments
dcbartlett
·
10Comments
m4a1fox
·
11Comments