Skip to content
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
14 changes: 9 additions & 5 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ exports._normalizeConnectArgs = normalizeConnectArgs;
// called when creating new Socket, or when re-using a closed Socket
function initSocketHandle(self) {
self.destroyed = false;
self.bytesRead = 0;
self._bytesDispatched = 0;
self._sockname = null;

Expand Down Expand Up @@ -179,6 +178,9 @@ function Socket(options) {
// Reserve properties
this.server = null;
this._server = null;

// Used after `.destroy()`
this._bytesRead = 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider making this a symbol.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ack.

}
util.inherits(Socket, stream.Duplex);

Expand Down Expand Up @@ -470,6 +472,9 @@ Socket.prototype._destroy = function(exception, cb) {
if (this !== process.stderr)
debug('close handle');
var isException = exception ? true : false;
// `bytesRead` should be accessible after `.destroy()`
this._bytesRead = this._handle.bytesRead;

this._handle.close(() => {
debug('emit close');
this.emit('close', isException);
Expand Down Expand Up @@ -521,10 +526,6 @@ function onread(nread, buffer) {
// will prevent this from being called again until _read() gets
// called again.

// if it's not enough data, we'll just call handle.readStart()
// again right away.
self.bytesRead += nread;

// Optimization: emit the original buffer with end points
var ret = self.push(buffer);

Expand Down Expand Up @@ -580,6 +581,9 @@ Socket.prototype._getpeername = function() {
return this._peername;
};

Socket.prototype.__defineGetter__('bytesRead', function() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since __defineGetter__ is deprecated now, can these be turned into proper Object.defineProperty() calls? Perhaps as a separate commit in this PR?

e.g.

Object.defineProperty(Socket.prototype, 'bytesRead', {
  configurable: false,
  enumerable: true,
  get: function() {
    return this._handle ? this.handle.bytesRead : this[BYTES_READ];
  }
});

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ack.

return this._handle ? this._handle.bytesRead : this._bytesRead;
});

Socket.prototype.__defineGetter__('remoteAddress', function() {
return this._getpeername().address;
Expand Down
1 change: 1 addition & 0 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ namespace node {
V(buffer_string, "buffer") \
V(bytes_string, "bytes") \
V(bytes_parsed_string, "bytesParsed") \
V(bytes_read_string, "bytesRead") \
V(cached_data_string, "cachedData") \
V(cached_data_produced_string, "cachedDataProduced") \
V(cached_data_rejected_string, "cachedDataRejected") \
Expand Down
16 changes: 16 additions & 0 deletions src/stream_base-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ void StreamBase::AddMethods(Environment* env,
v8::DEFAULT,
attributes);

t->InstanceTemplate()->SetAccessor(env->bytes_read_string(),
GetBytesRead<Base>,
nullptr,
env->as_external(),
v8::DEFAULT,
attributes);

env->SetProtoMethod(t, "readStart", JSMethod<Base, &StreamBase::ReadStart>);
env->SetProtoMethod(t, "readStop", JSMethod<Base, &StreamBase::ReadStop>);
if ((flags & kFlagNoShutdown) == 0)
Expand Down Expand Up @@ -79,6 +86,15 @@ void StreamBase::GetFD(Local<String> key,
}


template <class Base>
void StreamBase::GetBytesRead(Local<String> key,
const PropertyCallbackInfo<Value>& args) {
StreamBase* wrap = Unwrap<Base>(args.Holder());

args.GetReturnValue().Set(wrap->bytes_read_);
}


template <class Base>
void StreamBase::GetExternal(Local<String> key,
const PropertyCallbackInfo<Value>& args) {
Expand Down
13 changes: 11 additions & 2 deletions src/stream_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class StreamResource {
uv_handle_type pending,
void* ctx);

StreamResource() {
StreamResource() : bytes_read_(0) {
}
virtual ~StreamResource() = default;

Expand All @@ -160,9 +160,11 @@ class StreamResource {
alloc_cb_.fn(size, buf, alloc_cb_.ctx);
}

inline void OnRead(size_t nread,
inline void OnRead(ssize_t nread,
const uv_buf_t* buf,
uv_handle_type pending = UV_UNKNOWN_HANDLE) {
if (nread > 0)
bytes_read_ += nread;
if (!read_cb_.is_empty())
read_cb_.fn(nread, buf, pending, read_cb_.ctx);
}
Expand All @@ -182,6 +184,9 @@ class StreamResource {
Callback<AfterWriteCb> after_write_cb_;
Callback<AllocCb> alloc_cb_;
Callback<ReadCb> read_cb_;
int bytes_read_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why int instead of size_t?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Come to think of it, it should be uint64_t. Both int and size_t can overflow after 2 or 4 GB, which isn't out of the ordinary.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 to uint_64, int would likely overflow very quickly

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ack, with some explicit conversions.


friend class StreamBase;
};

class StreamBase : public StreamResource {
Expand Down Expand Up @@ -249,6 +254,10 @@ class StreamBase : public StreamResource {
static void GetExternal(v8::Local<v8::String> key,
const v8::PropertyCallbackInfo<v8::Value>& args);

template <class Base>
static void GetBytesRead(v8::Local<v8::String> key,
const v8::PropertyCallbackInfo<v8::Value>& args);

template <class Base,
int (StreamBase::*Method)( // NOLINT(whitespace/parens)
const v8::FunctionCallbackInfo<v8::Value>& args)>
Expand Down
38 changes: 38 additions & 0 deletions test/parallel/test-net-bytes-read.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const net = require('net');

const big = new Buffer(1024 * 1024);

@jasnell jasnell Apr 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Buffer.allocUnsafe(1024 * 1024)

actually...

Buffer.alloc(1024 * 1024, '-');

and you can remove the fill on the next line

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mmm... this may give us some headache during backport to v4

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nevertheless, ack.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Understood. We're going to have that problem in general anyway. Best to use the new APIs moving forward tho

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, that's why I fixed it ;)

big.fill('-');

const server = net.createServer((socket) => {
socket.end(big);
server.close();
}).listen(common.PORT, () => {
let prev = 0;

function checkRaise(value) {
assert(value > prev);
prev = value;
}

const socket = net.connect(common.PORT, () => {
socket.on('data', (chunk) => {
checkRaise(socket.bytesRead);
});

socket.on('end', common.mustCall(() => {
assert.equal(socket.bytesRead, prev);
assert.equal(big.length, prev);
}));

socket.on('close', common.mustCall(() => {
assert(!socket._handle);
assert.equal(socket.bytesRead, prev);
assert.equal(big.length, prev);
}));
});
socket.end();
});