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

stream_base: expose bytesRead getter #6284

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
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
18 changes: 13 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 All @@ -112,6 +111,10 @@ function initSocketHandle(self) {
}
}


const BYTES_READ = Symbol('bytesRead');
Copy link
Member

Choose a reason for hiding this comment

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

I think the naming preference is something like kBytesRead

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it?

lib/readline.js:const KEYPRESS_DECODER = Symbol('keypress-decoder');
lib/readline.js:const ESCAPE_DECODER = Symbol('escape-decoder');
lib/repl.js:exports.REPL_MODE_SLOPPY = Symbol('repl-sloppy');
lib/repl.js:exports.REPL_MODE_STRICT = Symbol('repl-strict');
lib/repl.js:exports.REPL_MODE_MAGIC = Symbol('repl-magic');

Copy link
Member

Choose a reason for hiding this comment

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

Ok, yep, I've seen comments both ways but the code doesn't lie ;-)



function Socket(options) {
if (!(this instanceof Socket)) return new Socket(options);

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

// Used after `.destroy()`
this[BYTES_READ] = 0;
}
util.inherits(Socket, stream.Duplex);

Expand Down Expand Up @@ -470,6 +476,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[BYTES_READ] = this._handle.bytesRead;

this._handle.close(() => {
debug('emit close');
this.emit('close', isException);
Expand Down Expand Up @@ -521,10 +530,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 +585,9 @@ Socket.prototype._getpeername = function() {
return this._peername;
};

Socket.prototype.__defineGetter__('bytesRead', function() {
Copy link
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
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[BYTES_READ];
});

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
17 changes: 17 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,16 @@ 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());

// int64_t -> double. 53bits is enough for all real cases.
Copy link
Member

Choose a reason for hiding this comment

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

uint64_t -> double

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, thanks!

args.GetReturnValue().Set(static_cast<double>(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_ += static_cast<uint64_t>(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_;
uint64_t bytes_read_;

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);
Copy link
Member

@jasnell jasnell Apr 20, 2016

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
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
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
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
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();
});