stream,net: slightly more consistent typechecking by addaleax · Pull Request #17644 · nodejs/node

addaleax · 2017-12-13T06:43:40Z

stream: remove undefined check

validChunk allowed undefined as a chunk in object mode; however,
this was redundant, since:
- validChunk() is only used by .write()
- If the validChunk() check passes for undefined, .write()
  calls writeOrBuffer()
- If writeOrBuffer() does not receive a Buffer, it calls
  decodeChunk()
  - decodeChunk() ignores undefined because it checks
    typeof chunk === 'string'
  - After that call, chunk.length is accessed, which throws an
    error if chunk is undefined.
This “fixes” a bug in the sense that state.pendingcb is no longer
incremented for write attempts that fail like this.
net: remove Socket.prototype.write

This is superfluous now that typechecking in net and
stream are aligned.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included (test-net-connect-buffer checks the error for undefined)
commit message follows commit guidelines

Affected core subsystem(s)

net, stream

`validChunk` allowed `undefined` as a chunk in object mode; however, this was redundant, since: - `validChunk()` is only used by `.write()` - If the `validChunk()` check passes for `undefined`, `.write()` calls `writeOrBuffer()` - If `writeOrBuffer()` does not receive a Buffer, it calls `decodeChunk()` - `decodeChunk()` ignores `undefined` because it checks `typeof chunk === 'string'` - After that call, `chunk.length` is accessed, which throws an error if `chunk` is undefined`. This “fixes” a bug in the sense that `state.pendingcb` is no longer incremented for write attempts that fail like this.

This is superfluous now that typechecking in `net` and `stream` are aligned.

addaleax · 2017-12-13T06:47:47Z

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

I would consider this semver-patch – it essentially turns a Cannot read property 'length' of undefined error into a proper type checking error, and leaves pendingcb intact. Without the latter, such a write would otherwise leave the stream hanging, so it seems very safe to assume that nobody relies on the previous behavior.

BridgeAR

LGTM but adding a test would be good.

BridgeAR · 2017-12-13T14:45:16Z

+1 on semver-patch by the way.

addaleax · 2017-12-13T16:33:50Z

@BridgeAR There are tests that cover the typechecking for net.Socket (which, granted, could use a refactor), so the second commit enables those tests as tests for the first one. :)

BridgeAR · 2017-12-15T21:38:52Z

Landed in 6918fff, 19b4485

`validChunk` allowed `undefined` as a chunk in object mode; however, this was redundant, since: - `validChunk()` is only used by `.write()` - If the `validChunk()` check passes for `undefined`, `.write()` calls `writeOrBuffer()` - If `writeOrBuffer()` does not receive a Buffer, it calls `decodeChunk()` - `decodeChunk()` ignores `undefined` because it checks `typeof chunk === 'string'` - After that call, `chunk.length` is accessed, which throws an error if `chunk` is undefined`. This “fixes” a bug in the sense that `state.pendingcb` is no longer incremented for write attempts that fail like this. PR-URL: nodejs#17644 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

This is superfluous now that typechecking in `net` and `stream` are aligned. PR-URL: nodejs#17644 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

`validChunk` allowed `undefined` as a chunk in object mode; however, this was redundant, since: - `validChunk()` is only used by `.write()` - If the `validChunk()` check passes for `undefined`, `.write()` calls `writeOrBuffer()` - If `writeOrBuffer()` does not receive a Buffer, it calls `decodeChunk()` - `decodeChunk()` ignores `undefined` because it checks `typeof chunk === 'string'` - After that call, `chunk.length` is accessed, which throws an error if `chunk` is undefined`. This “fixes” a bug in the sense that `state.pendingcb` is no longer incremented for write attempts that fail like this. PR-URL: #17644 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

This is superfluous now that typechecking in `net` and `stream` are aligned. PR-URL: #17644 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

`validChunk` allowed `undefined` as a chunk in object mode; however, this was redundant, since: - `validChunk()` is only used by `.write()` - If the `validChunk()` check passes for `undefined`, `.write()` calls `writeOrBuffer()` - If `writeOrBuffer()` does not receive a Buffer, it calls `decodeChunk()` - `decodeChunk()` ignores `undefined` because it checks `typeof chunk === 'string'` - After that call, `chunk.length` is accessed, which throws an error if `chunk` is undefined`. This “fixes” a bug in the sense that `state.pendingcb` is no longer incremented for write attempts that fail like this. PR-URL: #17644 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

This is superfluous now that typechecking in `net` and `stream` are aligned. PR-URL: #17644 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me>

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

Notable change: * async_hooks: - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither API were documented. (Andreas Madsen) #16972 * deps: - update nghttp2 to 1.29.0 (James M Snell) #17908 - upgrade npm to 5.6.0 (Kat Marchán) #17535 - cherry-pick 50f7455 from upstream V8 (Michaël Zasso) #16591 * events: - remove reaches into _events internals (Anatoli Papirovski) #17440 * http: - add rawPacket in err of `clientError` event (XadillaX) #17672 * http2: - implement maxSessionMemory (James M Snell) #17967 - add initial support for originSet (James M Snell) #17935 - add altsvc support (James M Snell) #17917 - perf_hooks integration (James M Snell) #17906 - Refactoring and cleanup of Http2Session and Http2Stream destroy (James M Snell) #17406 * net: - remove Socket.prototype.write (Anna Henningsen) #17644 - remove Socket.prototype.listen (Ruben Bridgewater) #13735 * repl: - show lexically scoped vars in tab completion (Michaël Zasso) #16591 * stream: - rm {writeable/readable}State.length (Calvin Metcalf) #12857 - add flow and buffer properties to streams (Calvin Metcalf) #12855 * util: - allow wildcards in NODE_DEBUG variable (Tyler) #17609 * zlib: - add ArrayBuffer support (Jem Bezooyen) #16042 * Addedew collaborator** - [starkwang](https://github.com/starkwang) Weijia Wang * Addedew TSC member** - [danbev](https://github.com/danbev) Daniel Bevenius PR-URL: #18069

addaleax added 2 commits December 13, 2017 07:34

net: remove Socket.prototype.write

1a7918c

This is superfluous now that typechecking in `net` and `stream` are aligned.

nodejs-github-bot added net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels Dec 13, 2017

BridgeAR approved these changes Dec 13, 2017

View reviewed changes

cjihrig approved these changes Dec 13, 2017

View reviewed changes

lpinca approved these changes Dec 13, 2017

View reviewed changes

apapirovski approved these changes Dec 13, 2017

View reviewed changes

jasnell approved these changes Dec 13, 2017

View reviewed changes

maclover7 approved these changes Dec 13, 2017

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 Dec 15, 2017

BridgeAR closed this Dec 15, 2017

addaleax added dont-land-on-v6.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 23, 2017

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

stream,net: slightly more consistent typechecking#17644

stream,net: slightly more consistent typechecking#17644
addaleax wants to merge 2 commits intonodejs:masterfrom
addaleax:no-socket-write

addaleax commented Dec 13, 2017 •

edited

Loading

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

BridgeAR left a comment

Uh oh!

BridgeAR commented Dec 13, 2017

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

addaleax commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 13, 2017

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleax commented Dec 13, 2017 •

edited

Loading