child_process: fix leak when passing http sockets

[santigimeno]

After passing an HTTP socket, release its associated resources.

Fixes: #15651

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[santigimeno]

/cc @gireeshpunathil @nodejs/child_process

CI: https://ci.nodejs.org/job/node-test-pull-request/14500/

[gireeshpunathil]

thanks, will test and let you know.

[gireeshpunathil]

Unfortunately I am unable to see any difference:

without the changes: (I am printing only the usedHeap value, because that is the one which matters)

4958728
5015128
5022584
5024952
5027112
5029272
5031432
5033520
5035608
5299920
^C

with the change:

4948888
5005288
5012744
5015112
5017272
5019432
5021592
5023680
5025768
5290080

[santigimeno]

@gireeshpunathil What test code are you using? Thanks

[gireeshpunathil]

#cat s.js

const http = require('http');
const { fork } = require('child_process');

const server = http.createServer((req, res) => {
    const workerProcess = fork('worker.js', [], {execArgv: []});
    workerProcess.on('message', (status) => {
        switch (status) {
            case 'ready':
                workerProcess.send({
                    method: req.method,
                    headers: req.headers,
                    path: req.path,
                    httpVersionMajor: req.httpVersionMajor,
                    query: req.query,
                }, res.socket);
                break;

            case 'done':
                res.socket.end();
        }
    });
});

server.listen(8080);

setInterval(() => {
  console.log(process.memoryUsage().heapUsed)
}, 10000);

#cat worker.js

const http = require('http');

process.on('message', (req, socket) => {
    const res = new http.ServerResponse(req);
    res._finish = function _finish() {
        res.emit('prefinish');
        socket.end();
    };
    res.assignSocket(socket);
    socket.once('finish', () => {
        process.send('done', () => {
            process.exit(0);
        });
    });
    res.end(process.pid.toString());
});

process.send('ready');

[santigimeno]

@gireeshpunathil I'm not seeing that behavior. With the patch, I've been running the code locally for an hour an the heapUsed never went above 3000000. On the other hand, with v10.0.0, after a few iterations the heap was exhausted:

> concurrently "node --max_old_space_size=32 s.js" "slam -t 3600 -c 50 http://localhost:8080"

[1] slam v1.2.0
[1] Thu Apr 26 2018 12:13:49 GMT+0200 (CEST)
[1] 
[1] slamming http://localhost:8080/ x50 for 3600s...
[0] 11422696
[0] 9437768
[0] 12545024
[0] 16679736
[0] 14676128
[0] 16663792
[0] 18578472
[0] 20249056
[0] 21852712
[0] 23683224
[0] 25397664
[0] 26996088
[0] 28509240
[0] 30519384
[0] 32254536
[0] 34025048
[0] 35271408
[0] 36720832
[0] 
[0] <--- Last few GCs --->
[0] 
[0] [20186:0x350c170]   192976 ms: Mark-sweep 36.1 (73.3) -> 36.1 (42.3) MB, 21.3 / 0.0 ms  (+ 0.0 ms in 0 steps since start of marking, biggest step 0.0 ms, walltime since start of marking 21 ms) last resort GC in old space requested
[0] [20186:0x350c170]   192999 ms: Mark-sweep 36.1 (42.3) -> 36.1 (42.3) MB, 23.8 / 0.0 ms  last resort GC in old space requested
[0] 
[0] 
[0] <--- JS stacktrace --->
[0] 
[0] ==== JS stack trace =========================================
[0] 
[0] Security context: 0x3185878a06a9 <JSObject>
[0]     0: builtin exit frame: parse(this=0x3185878886a9 <Object map = 0x3af1d1882ac1>,0x34980c91f561 <String[25]: {"cmd":"NODE_HANDLE_ACK"}>,0x3185878886a9 <Object map = 0x3af1d1882ac1>)
[0] 
[0]     1: onread [internal/child_process.js:495] [bytecode=0x20fa313f6c9 offset=167](this=0x34980c93baf9 <Pipe map = 0x3af1d18dbad1>,nread=33,pool=0x34980c91f471 <Uint8Array map = 0x14715a084d71>)
[0]     2: InternalFram...
[0] 
[0] FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory

P.S. I've been using your code along with the test runner in https://github.com/Reggino/node-issue-15651-test
P.P.S I had to remove the res.socket.end(); on the server code.

[gireeshpunathil]

I did further testing, and able to see what you saw:

• I wasn't starting a client at all, and was comparing the incremental increase in bytes due to the small objects that spin up due to console.log in the timer callback. With clients connecting, I see the difference.
• I added a gc() call in the timer callback, and see that the heap is back to normal all the time.

thanks, LGTM!

[BridgeAR]

Landed in 511230f 🎉