net: track bytesWritten in C++ land by addaleax · Pull Request #19551 · nodejs/node

addaleax · 2018-03-23T11:46:47Z

Move tracking of socket.bytesWritten to C++ land.

This makes it easier to provide this functionality for all
StreamBase instances, and in particular should keep working
when they have been 'consumed' in C++ in some way (e.g. for
the network sockets that are underlying to TLS or HTTP2 streams).

Also, this parallels socket.bytesRead a lot more now.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

addaleax · 2018-03-24T16:31:02Z

Added a regression test for #19562 + a bit more cleanup to make sure req.bytes always refers to the total length of the dispatched data

addaleax · 2018-03-24T16:34:47Z

~~CI: https://ci.nodejs.org/job/node-test-commit/17146/~~
CI: https://ci.nodejs.org/job/node-test-commit/17148/

addaleax · 2018-03-27T00:12:24Z

/cc @jasnell @apapirovski

addaleax · 2018-03-27T11:01:06Z

Re-building Linux CI: https://ci.nodejs.org/job/node-test-commit-linux/17383/

Move tracking of `socket.bytesWritten` to C++ land. This makes it easier to provide this functionality for all `StreamBase` instances, and in particular should keep working when they have been 'consumed' in C++ in some way (e.g. for the network sockets that are underlying to TLS or HTTP2 streams). Also, this parallels `socket.bytesRead` a lot more now.

Fixes: nodejs#19562

Simply always tell the caller how many bytes were written, rather than letting them track it. In the case of writing a string, also keep track of the bytes written by the earlier `DoTryWrite()`. Refs: nodejs#19562

addaleax · 2018-03-29T21:39:46Z

Rebased

CI: https://ci.nodejs.org/job/node-test-commit/17272/

addaleax · 2018-03-30T12:28:17Z

Landed in abc8786...b7cfd27

Move tracking of `socket.bytesWritten` to C++ land. This makes it easier to provide this functionality for all `StreamBase` instances, and in particular should keep working when they have been 'consumed' in C++ in some way (e.g. for the network sockets that are underlying to TLS or HTTP2 streams). Also, this parallels `socket.bytesRead` a lot more now. PR-URL: #19551 Reviewed-By: James M Snell <jasnell@gmail.com>

Fixes: #19562 PR-URL: #19551 Reviewed-By: James M Snell <jasnell@gmail.com>

Simply always tell the caller how many bytes were written, rather than letting them track it. In the case of writing a string, also keep track of the bytes written by the earlier `DoTryWrite()`. Refs: #19562 PR-URL: #19551 Reviewed-By: James M Snell <jasnell@gmail.com>

targos · 2018-04-02T13:54:16Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Move tracking of `socket.bytesWritten` to C++ land. This makes it easier to provide this functionality for all `StreamBase` instances, and in particular should keep working when they have been 'consumed' in C++ in some way (e.g. for the network sockets that are underlying to TLS or HTTP2 streams). Also, this parallels `socket.bytesRead` a lot more now. PR-URL: nodejs#19551 Reviewed-By: James M Snell <jasnell@gmail.com>

Fixes: nodejs#19562 PR-URL: nodejs#19551 Reviewed-By: James M Snell <jasnell@gmail.com>

Simply always tell the caller how many bytes were written, rather than letting them track it. In the case of writing a string, also keep track of the bytes written by the earlier `DoTryWrite()`. Refs: nodejs#19562 PR-URL: nodejs#19551 Reviewed-By: James M Snell <jasnell@gmail.com>

Fixes: #19562 PR-URL: #19551 Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 23, 2018

This was referenced Mar 24, 2018
src: refactor WriteWrap and ShutdownWrap #18676
Closed
Regression: Wrong value in bytesWritten for net.Socket #19562
Closed

addaleax force-pushed the bytes-written branch from 5e7bf1c to 0ca4f45Compare March 24, 2018 18:24

jasnell approved these changes Mar 27, 2018
View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 27, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 28, 2018

addaleax added 4 commits March 29, 2018 23:21

[squash] fix windows 🤞
0c5dea8

test: add regression test for large write
0b824ed
Fixes: nodejs#19562

src: clean up req.bytes tracking
6f6f00f
Simply always tell the caller how many bytes were written, rather than letting them track it. In the case of writing a string, also keep track of the bytes written by the earlier `DoTryWrite()`. Refs: nodejs#19562

addaleax force-pushed the bytes-written branch from 0ca4f45 to 6f6f00fCompare March 29, 2018 21:21

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2018

addaleax closed this Mar 30, 2018

addaleax added a commit that referenced this pull request Mar 30, 2018
test: add regression test for large write
1dc8eb4
Fixes: #19562 PR-URL: #19551 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax deleted the bytes-written branch March 30, 2018 12:29

addaleax mentioned this pull request Apr 1, 2018
net,http2: merge write error handling & property names #19734
Closed
2 tasks

targos added backport-requested-v9.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 2, 2018

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
test: add regression test for large write
2bcd795
Fixes: nodejs#19562 PR-URL: nodejs#19551 Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Jun 29, 2018
test: add regression test for large write
4f7f003
Fixes: #19562 PR-URL: #19551 Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Jul 9, 2018
v8.12.0 proposal #21593
Merged

rvagg pushed a commit that referenced this pull request Aug 16, 2018
test: add regression test for large write
3c6464a
Fixes: #19562 PR-URL: #19551 Reviewed-By: James M Snell <jasnell@gmail.com>

vtjnash mentioned this pull request Jan 13, 2020
win: pipe UV_HANDLE_EMULATE_IOCP implementation broken libuv/libuv#629
Closed

Navigation Menu

Uh oh!

Uh oh!

net: track bytesWritten in C++ land - #19551

net: track bytesWritten in C++ land#19551
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:bytes-written

addaleax commented Mar 23, 2018

Uh oh!

addaleax commented Mar 24, 2018

Uh oh!

addaleax commented Mar 24, 2018•
edited
Loading

Uh oh!

addaleax commented Mar 27, 2018

Uh oh!

addaleax commented Mar 27, 2018

Uh oh!

addaleax commented Mar 29, 2018

Uh oh!

addaleax commented Mar 30, 2018

Uh oh!

targos commented Apr 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

addaleax commented Mar 23, 2018

Checklist

Uh oh!

addaleax commented Mar 24, 2018

Uh oh!

addaleax commented Mar 24, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Mar 27, 2018

Uh oh!

addaleax commented Mar 27, 2018

Uh oh!

addaleax commented Mar 29, 2018

Uh oh!

addaleax commented Mar 30, 2018

Uh oh!

targos commented Apr 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

addaleax commented Mar 24, 2018•
edited
Loading