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

Memory leak with http2 write() #18169

Closed
dolanmiu opened this issue Jan 16, 2018 · 6 comments
Closed

Memory leak with http2 write() #18169

dolanmiu opened this issue Jan 16, 2018 · 6 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.

Comments

@dolanmiu
Copy link

dolanmiu commented Jan 16, 2018

  • Version: 9.4.0
  • Platform: Windows 10 64-bit
  • Subsystem: http2

If you use req.write("") in http2, with an empty string, the memory gets higher and higher without end. Memory leak?

        req.write("", () => {
            console.log("dun knoe");
        });

image

Note: Re-creation script is a few posts down this one

@jasnell
Copy link
Member

jasnell commented Jan 16, 2018

Entirely possible. Will investigate.

ping /cc @addaleax

@jasnell jasnell added the http2 Issues or PRs related to the http2 subsystem. label Jan 16, 2018
@jasnell
Copy link
Member

jasnell commented Jan 16, 2018

Evaluating test-http2-zero-length-write.js is not revealing any leaks when using the lower level API. @dolanmiu ... can I ask you to please put together a more complete test case that demonstrates the issue consistently that I can try. Thank you.

@addaleax
Copy link
Member

the memory gets higher and higher without end. Memory leak?

How are you calling .write()? If you do so in a synchronous loop, this is more or less expected, because the streams implementation needs to keep track of each callback that you passed to write().

@dolanmiu
Copy link
Author

dolanmiu commented Jan 16, 2018

Here, I have created a simplified script to re-create the issue:

save it as a test.js file, and type node test.js:

const http2 = require("http2");

const client = http2.connect(`https://www.google.com`);

const req = client.request({
    ":method": "POST",
    ":path": `/v2/events`,
    authorization: `Bearer test-here`,
    "content-type": "multipart/form-data; boundary=dench-gang",
});

req.on("response", (headers, flags) => {
    for (const name in headers) {
        console.log(`${name}: ${headers[name]}`);
    }
});

req.write("");

req.end();

@dolanmiu
Copy link
Author

@addaleax no loop from what I can see, unless I am missing something

@XadillaX
Copy link
Contributor

XadillaX commented Feb 9, 2018

I think I've found the key. I'll work on it.

@XadillaX XadillaX self-assigned this Feb 9, 2018
@BridgeAR BridgeAR added the confirmed-bug Issues with confirmed bugs. label Feb 9, 2018
XadillaX added a commit to XadillaX/node that referenced this issue Feb 11, 2018
If we do `req.write("")` (an empty string) on Http2Stream, the gather
logic will be messed up:

```
while ((src_length = nghttp2_session_mem_send(session_, &src)) > 0) {
  printf("write\n");
  DEBUG_HTTP2SESSION2(this, "nghttp2 has %d bytes to send", src_length);
  CopyDataIntoOutgoing(src, src_length);
}
```

The logic above is in function `Http2Session::SendPendingData` under
src/node_http2.cc. This `while` will be endless when an empty string is
inside because `src_length` will always be 9.

And then the main event loop thread will be blocked and the memory used
will be larger and larger.

This pull request is to ignore empty string or buffer in `_write()` and
`_writev()` of Http2Stream.

Fixes: nodejs#18169
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
addaleax added a commit to addaleax/node that referenced this issue Mar 5, 2018
PR-URL: nodejs#18924
Fixes: nodejs#18169
Refs: nodejs#18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
kjin pushed a commit to kjin/node that referenced this issue May 1, 2018
PR-URL: nodejs#18924
Fixes: nodejs#18169
Refs: nodejs#18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue May 2, 2018
Backport-PR-URL: #20456
PR-URL: #18924
Fixes: #18169
Refs: #18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18924
Fixes: nodejs#18169
Refs: nodejs#18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue May 15, 2018
Backport-PR-URL: #20456
PR-URL: #18924
Fixes: #18169
Refs: #18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue May 15, 2018
Backport-PR-URL: #20456
PR-URL: #18924
Fixes: #18169
Refs: #18673
Refs: https://github.com/nodejs/node/blob/v9.5.0/src/node_http2.cc#L1481-L1484
Refs: https://github.com/nodejs/node/blob/v9.5.0/lib/_http_outgoing.js#L659-L661
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants