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

net: improve socket.write() error message #5981

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@phillipj
Member

phillipj commented Mar 31, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

net

Description of change

Informative error messages are very important for developers and could possibly save hours of debugging and frustration. This improves the error message thrown when writing invalid data into a socket, by communicating what's expected compared to what the developer just tried to write.

When passing anything other than a string or buffer to socket.write(), a TypeError with saying Invalid data gets thrown. E.g:

TypeError: Invalid data
    at Socket.write (net.js:623:11)
    at Socket.net.createConnection (/Users/phillipj/my-awesome-sock-script.js:5:12)
    at Socket.g (events.js:271:16)
    at emitNone (events.js:85:20)
    at Socket.emit (events.js:179:7)

This has bit me more than once as it doesn't really say much about what was expected or what I did wrong, other than some data being invalid.

Getting something like this would be a lot more helpful:

TypeError: Invalid data, chunk must be a string or a buffer, not number
    at Socket.write (net.js:623:11)
    at Socket.net.createConnection (/Users/phillipj/my-awesome-sock-script.js:5:12)
    at Socket.g (events.js:271:16)
    at emitNone (events.js:85:20)
    at Socket.emit (events.js:179:7)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1048:10)```
net: improve socket.write() error message
Informative error messages are very important for developers and could
possibly save hours of debugging and frustration. This improves the error
message thrown when writing invalid data into a socket, by communicating
what's expected compared to what the developer just tried to write.

@phillipj phillipj added the net label Mar 31, 2016

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Mar 31, 2016

Major, since error messages are currently their only identifier.

@mscdex

This comment has been minimized.

Contributor

mscdex commented Mar 31, 2016

LGTM

@phillipj

This comment has been minimized.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Mar 31, 2016

LGTM

1 similar comment
@jasnell

This comment has been minimized.

Member

jasnell commented Apr 1, 2016

LGTM

jasnell added a commit that referenced this pull request Apr 1, 2016

net: improve socket.write() error message
Informative error messages are very important for developers and could
possibly save hours of debugging and frustration. This improves the error
message thrown when writing invalid data into a socket, by communicating
what's expected compared to what the developer just tried to write.

PR-URL: #5981
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Apr 1, 2016

Landed in ec49fc8

@jasnell jasnell closed this Apr 1, 2016

@phillipj phillipj deleted the phillipj:improve-socket-write-err branch Apr 3, 2016

@jasnell jasnell referenced this pull request Apr 19, 2016

Closed

What is new in v6? #6264

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

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

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

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

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

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

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

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).

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

2016-04-26, Version 6.0.0 (Current) Release
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.

* Buffer
  * New Buffer constructors have been added
    [#4682](#4682)
  * Previously deprecated Buffer APIs are removed
    [#5048](#5048),
    [#4594](#4594)
  * Improved error handling [#4514](#4514)
* Cluster
  * Worker emitted as first argument in 'message' event
    [#5361](#5361).
* Crypto
  * Improved error handling [#3100](#3100),
    [#5611](#5611)
  * Simplified Certificate class bindings
    [#5382](#5382)
  * Improved control over FIPS mode
    [#5181](#5181)
  * pbkdf2 digest overloading is deprecated
    [#4047](#4047)
* Dependencies
  * Reintroduce shared c-ares build support
    [#5775](#5775).
  * V8 updated to 5.0.71.31 [#6111](#6111).
* DNS
  * Add resolvePtr API to query plain DNS PTR records
    [#4921](#4921).
* Domains
  * Clear stack when no error handler
  [#4659](#4659).
* File System
  * The `fs.realpath()` and `fs.realpathSync()` methods have been updated
    to use a more efficient libuv implementation. This change includes the
    removal of the `cache` argument and the method can throw new errors
    [#3594](#3594)
  * FS apis can now accept and return paths as Buffers
    [#5616](#5616).
  * Error handling and type checking improvements
    [#5616](#5616),
    [#5590](#5590),
    [#4518](#4518),
    [#3917](#3917).
  * fs.read's string interface is deprecated
    [#4525](#4525)
* HTTP
  * 'clientError' can now be used to return custom errors from an
    HTTP server [#4557](#4557).
* Modules
  * Current directory is now prioritized for local lookups
    [#5689](#5689)
  * Symbolic links are preserved when requiring modules
    [#5950](#5950)
* Net
  * DNS hints no longer implicitly set
    [#6021](#6021).
  * Improved error handling and type checking
    [#5981](#5981),
    [#5733](#5733),
    [#2904](#2904)
* OS X
  * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
    [#6402](#6402).
* Path
  * Improved type checking [#5348](#5348).
* Process
  * Introduce process warnings API
    [#4782](#4782).
  * Throw exception when non-function passed to nextTick
    [#3860](#3860).
* Readline
  * Emit key info unconditionally
    [#6024](#6024)
* REPL
  * Assignment to `_` will emit a warning.
    [#5535](#5535)
* Timers
  * Fail early when callback is not a function
    [#4362](#4362)
* TLS
  * Rename 'clientError' to 'tlsClientError'
    [#4557](#4557)
  * SHA1 used for sessionIdContext
    [#3866](#3866)
* TTY
  * Previously deprecated setRawMode wrapper is removed
    [#2528](#2528).
* Util
  * Changes to Error object formatting
    [#4582](#4582).
* Windows
  * Windows XP and Vista are no longer supported
    [#5167](#5167),
    [#5167](#5167).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment