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

net: make holding the buffer in memory more robust #8252

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,6 @@ Socket.prototype._writeGeneric = function(writev, data, encoding, cb) {
} else {
var enc;
if (data instanceof Buffer) {
req.buffer = data; // Keep reference alive.
enc = 'buffer';
} else {
enc = encoding;
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ namespace node {
V(args_string, "args") \
V(async, "async") \
V(async_queue_string, "_asyncQueue") \
V(buffer_string, "buffer") \
V(bytes_string, "bytes") \
V(bytes_parsed_string, "bytesParsed") \
V(bytes_read_string, "bytesRead") \
Expand Down
1 change: 1 addition & 0 deletions src/stream_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ int StreamBase::WriteBuffer(const FunctionCallbackInfo<Value>& args) {

err = DoWrite(req_wrap, bufs, count, nullptr);
req_wrap_obj->Set(env->async(), True(env->isolate()));
req_wrap_obj->Set(env->buffer_string(), args[1]);

if (err)
req_wrap->Dispose();
Expand Down
34 changes: 34 additions & 0 deletions test/parallel/test-net-write-fully-async-buffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';
// Flags: --expose-gc

// Note: This is a variant of test-net-write-fully-async-hex-string.js.
// This always worked, but it seemed appropriate to add a test that checks the
// behaviour for Buffers, too.
const common = require('../common');
const net = require('net');

const data = Buffer.allocUnsafe(1000000);

const server = net.createServer(common.mustCall(function(conn) {
conn.resume();
})).listen(0, common.mustCall(function() {
const conn = net.createConnection(this.address().port, common.mustCall(() => {
let count = 0;

function write_loop() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/write_loop/writeLoop/ here and in the other file?

if (count++ === 200) {
conn.destroy();
server.close();
return;
}

while (conn.write(Buffer.from(data)));
global.gc(true);
// The buffer allocated above should still be alive.
}

conn.on('drain', write_loop);

write_loop();
}));
}));
32 changes: 32 additions & 0 deletions test/parallel/test-net-write-fully-async-hex-string.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
'use strict';
// Flags: --expose-gc

// Regression test for https://github.com/nodejs/node/issues/8251.
const common = require('../common');
const net = require('net');

const data = Buffer.allocUnsafe(1000000).toString('hex');

const server = net.createServer(common.mustCall(function(conn) {
conn.resume();
})).listen(0, common.mustCall(function() {
const conn = net.createConnection(this.address().port, common.mustCall(() => {
let count = 0;

function write_loop() {
if (count++ === 20) {
conn.destroy();
server.close();
return;
}

while (conn.write(data, 'hex'));
global.gc(true);
// The buffer allocated inside the .write() call should still be alive.
}

conn.on('drain', write_loop);

write_loop();
}));
}));