stream_base: expose `bytesRead` getter #6284

indutny · 2016-04-19T19:05:05Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

stream_base

Description of change

This will provide bytesRead data on consumed sockets.

Fix: #3021

This will provide `bytesRead` data on consumed sockets. Fix: nodejs#3021

indutny · 2016-04-19T19:06:38Z

cc @trevnorris

indutny · 2016-04-19T19:07:06Z

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

bnoordhuis · 2016-04-20T10:52:07Z

src/stream_base.h

@@ -182,6 +184,9 @@ class StreamResource {
  Callback<AfterWriteCb> after_write_cb_;
  Callback<AllocCb> alloc_cb_;
  Callback<ReadCb> read_cb_;
+  int bytes_read_;


Why int instead of size_t?

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.

+1 to uint_64, int would likely overflow very quickly

Ack, with some explicit conversions.

indutny · 2016-04-20T15:41:52Z

@bnoordhuis should be fixed now, PTAL

indutny · 2016-04-20T15:42:10Z

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

jasnell · 2016-04-20T15:42:16Z

lib/net.js

@@ -112,6 +111,10 @@ function initSocketHandle(self) {
  }
 }

+
+const BYTES_READ = Symbol('bytesRead');


I think the naming preference is something like kBytesRead

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');

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

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead.

indutny · 2016-04-20T16:01:01Z

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

bnoordhuis · 2016-04-20T16:25:25Z

src/stream_base-inl.h

+                              const PropertyCallbackInfo<Value>& args) {
+  StreamBase* wrap = Unwrap<Base>(args.Holder());
+
+  // int64_t -> double. 53bits is enough for all real cases.


uint64_t -> double

Ack, thanks!

bnoordhuis · 2016-04-20T16:26:09Z

LGTM with nit.

indutny · 2016-04-20T16:41:31Z

Unrelated failure on ARM:


Test Name
Duration
Age
 53 - test-http-agent.js

indutny · 2016-04-20T16:43:08Z

Landed in 6198472 and 8636af1, thank you!

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This will provide `bytesRead` data on consumed sockets. Fix: nodejs#3021 PR-URL: nodejs#6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: nodejs#6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2016-05-18T00:01:52Z

@indutny this isn't landing cleanly on v4.x... would you be able to backport?

indutny · 2016-05-20T19:48:15Z

Will do a backport.

This will provide `bytesRead` data on consumed sockets. Fix: nodejs#3021 PR-URL: nodejs#6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: nodejs#6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

indutny · 2016-05-20T19:53:06Z

@thealphanerd here you go #6903

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

stream_base: expose bytesRead getter

eb19df6

This will provide `bytesRead` data on consumed sockets. Fix: nodejs#3021

indutny mentioned this pull request Apr 19, 2016

http/server: request.socket.bytesRead always 0 for IncomingMessage #3021

Closed

mscdex added c++ Issues and PRs that require attention from people who are familiar with C++. net Issues and PRs related to the net subsystem. labels Apr 19, 2016

bnoordhuis reviewed Apr 20, 2016
View reviewed changes

indutny added 2 commits April 20, 2016 11:38

fixes

a6ee571

symbol

6357f46

jasnell reviewed Apr 20, 2016
View reviewed changes

indutny added the lts-watch-v4.x label Apr 20, 2016

indutny added 2 commits April 20, 2016 11:55

fix test

783cae2

net: replace __defineGetter__ with defineProperty

4c50cc4

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead.

bnoordhuis reviewed Apr 20, 2016
View reviewed changes

fix

62a88cb

indutny closed this Apr 20, 2016

indutny deleted the fix/gh-3021 branch April 20, 2016 16:43

indutny added a commit that referenced this pull request Apr 20, 2016

stream_base: expose bytesRead getter

6198472

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

indutny added a commit that referenced this pull request Apr 20, 2016

net: replace __defineGetter__ with defineProperty

8636af1

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Apr 20, 2016

stream_base: expose bytesRead getter

20dcdd3

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Apr 20, 2016

net: replace __defineGetter__ with defineProperty

d5d4f19

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016

stream_base: expose bytesRead getter

052d87c

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016

net: replace __defineGetter__ with defineProperty

36c58da

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

stream_base: expose bytesRead getter

4162dee

This will provide `bytesRead` data on consumed sockets. Fix: nodejs#3021 PR-URL: nodejs#6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell pushed a commit that referenced this pull request Apr 26, 2016

stream_base: expose bytesRead getter

479344f

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell pushed a commit that referenced this pull request Apr 26, 2016

net: replace __defineGetter__ with defineProperty

8db8a4b

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Toon34 mentioned this pull request Apr 29, 2016

ECONNRESET http-party/node-http-proxy#971

Open

MylesBorins assigned indutny May 18, 2016

indutny added a commit to indutny/io.js that referenced this pull request May 20, 2016

stream_base: expose bytesRead getter

f1c1355

This will provide `bytesRead` data on consumed sockets. Fix: nodejs#3021 PR-URL: nodejs#6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

indutny mentioned this pull request May 20, 2016

Backport of 6284 to v4.x #6903

Closed

4 tasks

MylesBorins pushed a commit that referenced this pull request Jun 1, 2016

stream_base: expose bytesRead getter

b6c17a0

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Jun 1, 2016

net: replace __defineGetter__ with defineProperty

ab2bfe0

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jun 1, 2016

MylesBorins pushed a commit that referenced this pull request Jun 23, 2016

stream_base: expose bytesRead getter

aee295b

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Jun 23, 2016

net: replace __defineGetter__ with defineProperty

ed09217

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Jun 24, 2016

stream_base: expose bytesRead getter

55319fe

This will provide `bytesRead` data on consumed sockets. Fix: #3021 PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins pushed a commit that referenced this pull request Jun 24, 2016

net: replace __defineGetter__ with defineProperty

b3acbc5

`Object.prototype.__defineGetter__` is deprecated now, use `Object.defineProperty` instead. PR-URL: #6284 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins mentioned this pull request Jun 24, 2016

v4.4.7 proposal #7395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream_base: expose `bytesRead` getter #6284

stream_base: expose `bytesRead` getter #6284

indutny commented Apr 19, 2016

indutny commented Apr 19, 2016

indutny commented Apr 19, 2016

bnoordhuis Apr 20, 2016

bnoordhuis Apr 20, 2016

jasnell Apr 20, 2016

indutny Apr 20, 2016

indutny commented Apr 20, 2016

indutny commented Apr 20, 2016

jasnell Apr 20, 2016

indutny Apr 20, 2016

jasnell Apr 20, 2016

indutny commented Apr 20, 2016

bnoordhuis Apr 20, 2016

indutny Apr 20, 2016

bnoordhuis commented Apr 20, 2016

indutny commented Apr 20, 2016

indutny commented Apr 20, 2016

MylesBorins commented May 18, 2016

indutny commented May 20, 2016

indutny commented May 20, 2016

stream_base: expose bytesRead getter #6284

stream_base: expose bytesRead getter #6284

Conversation

indutny commented Apr 19, 2016

Checklist

Affected core subsystem(s)

Description of change

indutny commented Apr 19, 2016

indutny commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 20, 2016

indutny commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Apr 20, 2016

indutny commented Apr 20, 2016

indutny commented Apr 20, 2016

MylesBorins commented May 18, 2016

indutny commented May 20, 2016

indutny commented May 20, 2016

stream_base: expose `bytesRead` getter #6284

stream_base: expose `bytesRead` getter #6284