Memory leak

[AlexBHarley]

Originally raised this in the netlify cli repository but I've tracked the issue down to here.

const express = require('express');
const serverless = require('serverless-http');

const app = express();
app.post('/.netlify/functions/index/health-check', (req, res) => {
    res.json({hello: Date.now()});
});

module.exports.handler = serverless(app);

Then I run the following script

const fetch = require('node-fetch');

(async function () {
  while (true) {
    const response = await fetch
      .default('http://localhost:8888/.netlify/functions/index/health-check', {
        method: 'POST',
        body: JSON.stringify(
          Array(500).fill({hello: 'there', date: Date.now()})
        ),
      })
      .then(x => x.json());
    console.log(response);
  }
})();

Consistently after ~20 seconds the process crashes with an out of memory error.

I'm pretty sure that it has something to do with the ReadableStream destroy/end methods never being called but haven't been able to fix it myself.

[dougmoscrop]

Yup, looks reproducible with just:

const serverless = require('serverless-http');

const handler = serverless((req, res) => {
    res.end('test');
});

async function run() {
    while (true) {
        await handler({
            path: '/',
            method: 'POST',
            body: 'hello'
        });
    }
}

run().catch(console.error);

[dougmoscrop]

The problem is at least in here: https://github.com/dougmoscrop/serverless-http/blob/master/lib/response.js#L93

If I don't attach the fake-writable-stream-socket the example above works fine, but many frameworks expect to be able to write to the socket directly, so I've screwed something up in there.

The other thing I did while testing was change ServerlessRequest to have a _read() method that pushes the body only when asked rather than right away.

Also a mistake on event listener removal: https://github.com/dougmoscrop/serverless-http/blob/master/lib/finish.js#L20

[dougmoscrop]

I could use some help ^ with whatever is going on in that onwrite bound event cycle if anybody has some time.

The "ServerResponse" class that we extend from to create the ServerlessResponse has a bunch of stuff in it that needs to be coordinated to make it look like a real server. Changing it to use http.OutgoingMessage instead is problematic for a few reasons: there's some helper code that would need to be copied or reimplemented, and some frameworks like express, mutate the prototype of the req/res anyway (!) -- I don't have a link to the code, but if you do app.callback()(req, res) -- the res becomes a http.ServerResponse even if what you sent in is an http.OutgoingMessage - I hunted down where this was happening in the express framework at one point, but just took it as "eh, use the http.ServerResponse" and stopped caring.

So I think the best way to fix this is stick with http.ServerResponse and figure out what's happening in .assignSocket() that isn't being correctly undone?

[dougmoscrop]

OK, I believe I have fixed this, but doing more testing

[dougmoscrop]

Should be fixed in 2.4.0, thanks a lot for reporting this, I would appreciate if you could confirm that you no longer see the issue.

[AlexBHarley]

Thanks for doing this so quickly, unfortunately just tested it out and still having trouble.

• Scenario 1 (original comment) is running with the netlify cli. And dies with an oom after 20-25 seconds
• Scenario 2 (your example) dies after ~16 seconds.

Unfortunately I don't have much more info to give you than that. I've moved towards just running an express server for development. Haven't gone live yet so I can't tell you how this is affecting our live env, don't have enough traffic yet to trigger the issue

[dougmoscrop]

What version of Node are you using?

[dougmoscrop]

Ah! I see it happening in 10, but not 12 (with 2.4.0). Interesting!

[AlexBHarley]

Yes sorry node 10. Netlify supports Node 12 so I guess my worries there are unfounded.

[dougmoscrop]

Yeah I'm not sure what the difference between 10 and 12 is, if I get cycles I can poke around, but if yo could try 2.4.1 on Node 12 that should work

[AlexBHarley]

Looks like out of the two scenarios I'm trying with Node v12.16.3

• Scenario 1: fails in 28.629 seconds
• Scenario 2: never fails! Even tried this one with bigger payloads.

It's definitely possible there's another issue lurking somewhere with netlify's cli tool or something else I'm doing. I'd chalk this up as a win

[dougmoscrop]

Okay thanks! The next time I get some cycles I will try it with netlify to see if I can spot anything.. the node http internals (and streams for that matter) involve a lot of wizardry and a lot of cross-mutation of private members so lots of ways I can see this having more issues, I tried to simplify it as best I could with a fake writable socket instead of an actual real stream, and I was feeling good about that until it didn't work with node 10!

[patrick91]

I've tried the first example on aws lamdba with node 12 and didn't get memory leaks (memory got up to 100mb max) :)