FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: fix leak when passing http sockets by santigimeno · Pull Request #20305 · nodejs/node · GitHub

/ node Public

child_process: fix leak when passing http sockets - #20305

Closed
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:child_leak
ConversationCommits (1)ChecksFiles changed
Closed

child_process: fix leak when passing http sockets#20305
santigimeno wants to merge 1 commit into
nodejs:masterfrom
santigimeno:child_leak

Conversation

santigimeno commented Apr 25, 2018

Copy link
Copy Markdown
Member

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

child_process: fix leak when passing http sockets
810d961 
After passing an HTTP socket, release its associated resources.

Fixes: nodejs#15651
nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Apr 25, 2018

santigimeno commented Apr 25, 2018

Copy link
Copy Markdown
Member Author

/cc @gireeshpunathil @nodejs/child_process

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

gireeshpunathil commented Apr 26, 2018

Copy link
Copy Markdown
Member

thanks, will test and let you know.

gireeshpunathil commented Apr 26, 2018

Copy link
Copy Markdown
Member

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 commented Apr 26, 2018

Copy link
Copy Markdown
Member Author

@gireeshpunathil What test code are you using? Thanks

gireeshpunathil commented Apr 26, 2018

Copy link
Copy Markdown
Member

#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 commented Apr 26, 2018

Copy link
Copy Markdown
Member Author

@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 commented Apr 26, 2018

Copy link
Copy Markdown
Member

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 29, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 29, 2018
child_process: fix leak when passing http sockets
511230f 
After passing an HTTP socket, release its associated resources.

PR-URL: nodejs#20305
Fixes: nodejs#15651
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR commented Apr 29, 2018

Copy link
Copy Markdown
Member

Landed in 511230f 🎉

BridgeAR closed this Apr 29, 2018
MylesBorins pushed a commit that referenced this pull request May 4, 2018
child_process: fix leak when passing http sockets
ec2037d 
After passing an HTTP socket, release its associated resources.

PR-URL: #20305
Fixes: #15651
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request May 8, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
gireeshpunathil gireeshpunathil approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. child_process Issues and PRs related to the child_process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory leak due to process.fork()?

5 participants

Back | FazBrowse Home | New Git URL