-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
benchmark: chunky http client benchmark variation and server #228
Conversation
var count = 0; | ||
var t0, tf; | ||
var socket = net.connect(PIPE, function() { | ||
bench.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: please indent with two spaces here.
Thanks for the great comments Ben they are all done! |
|
||
var server; | ||
|
||
fs.unlinkSync(PIPE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could throw with Error: ENOENT
. Instead do:
try {
fs.unlinkSync(PIPE);
} catch (e) { }
@@ -0,0 +1,104 @@ | |||
"use strict"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: the convention is to use single quotes. There is usually a blank line after 'use strict'.
child.on('error', function(err) { | ||
console.error('spawn error.'); | ||
console.error(err); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not going to handle them, you should let 'error' events bubble up.
Hi Trevor and Ben, I think the style is all set! Please let me know if you spot anything else I appreciate the practice. |
ping @trevnorris curious on your thoughts |
|
||
function main(conf) { | ||
var len = +conf.len; | ||
var num = +conf.num; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure conf.len
and conf.num
will always be defined? If not they'll return NaN
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this seems to be the assumption in all of the configuration code in the benchmark client headers net/ section. I am simply following that pre-existing convention. I did notice that there is a conf.len downstairs line 65 or so. I pushed (rebase -i) a minor cleanup fix for it to use 'len' instead. But both seem fine and correct to me so I doubt that this is more than a very minor aesthetic clarity issue.
One minor comment, but don't see anything I oppose here. |
I'm gonna bump this up and jump in. Hows this going? |
Fine. I'm happy to have this patch accepted when applicable. |
Seems you've addressed everything from Ben and myself. I'll LGTM. @bnoordhuis? |
ping @bnoordhuis it's been a couple weeks since anything has happened on this patch. may i ask if it is reasonable yet? |
@rudi-cilibrasi i'll give until monday for any other responses. if non come ping me and i'll land this. |
Sorry, the notification got lost in the gaping black hole that's my email backlog. I defer to @trevnorris; if he is happy with it, I'm happy with it. :-) |
no problems over the weekend. seems to be ready for acceptance. :-) On Fri, Mar 6, 2015 at 12:46 PM, Ben Noordhuis notifications@github.com
Happy to Code with Integrity : Software Engineering Code of Ethics and |
PR-URL: #228 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Thanks. Landed in 030a923. |
Full release notes: A few meaty bugfixes, and introducing `peerDependenciesMeta`. FEATURES * [`a12341088`](npm/cli@a123410) [nodejs#224](npm/cli#224) Implements peerDependenciesMeta ([@arcanis](https://github.com/arcanis)) * [`2f3b79bba`](npm/cli@2f3b79b) [nodejs#234](npm/cli#234) add new forbidden 403 error code ([@claudiahdz](https://github.com/claudiahdz)) BUGFIXES * [`24acc9fc8`](npm/cli@24acc9f) and [`45772af0d`](npm/cli@45772af) [nodejs#217](npm/cli#217) [npm.community#8863](https://npm.community/t/installing-the-same-module-under-multiple-relative-paths-fails-on-linux/8863) [npm.community#9327](https://npm.community/t/reinstall-breaks-after-npm-update-to-6-10-2/9327,) do not descend into directory deps' child modules, fix shrinkwrap files that inappropriately list child nodes of symlink packages ([@isaacs](https://github.com/isaacs) and [@salomvary](https://github.com/salomvary)) * [`50cfe113d`](npm/cli@50cfe11) [nodejs#229](npm/cli#229) fixed typo in semver doc ([@gall0ws](https://github.com/gall0ws)) * [`e8fb2a1bd`](npm/cli@e8fb2a1) [nodejs#231](npm/cli#231) Fix spelling mistakes in CHANGELOG-3.md ([@XhmikosR](https://github.com/XhmikosR)) * [`769d2e057`](npm/cli@769d2e0) [npm/uid-number#7](npm/uid-number#7) Better error on invalid `--user`/`--group` configs. This addresses the issue when people fail to install binary packages on Docker and other environments where there is no 'nobody' user. ([@isaacs](https://github.com/isaacs)) * [`8b43c9624`](npm/cli@8b43c96) [nodejs#28987](nodejs#28987) [npm.community#6032](https://npm.community/t/npm-ci-doesnt-respect-npmrc-variables/6032) [npm.community#6658](https://npm.community/t/npm-ci-doesnt-fill-anymore-the-process-env-npm-config-cache-variable-on-post-install-scripts/6658) [npm.community#6069](https://npm.community/t/npm-ci-does-not-compile-native-dependencies-according-to-npmrc-configuration/6069) [npm.community#9323](https://npm.community/t/npm-6-9-x-not-passing-environment-to-node-gyp-regression-from-6-4-x/9323/2) Fix the regression where random config values in a .npmrc file are not passed to lifecycle scripts, breaking build processes which rely on them. ([@isaacs](https://github.com/isaacs)) * [`8b85eaa47`](npm/cli@8b85eaa) save files with inferred ownership rather than relying on `SUDO_UID` and `SUDO_GID`. ([@isaacs](https://github.com/isaacs)) * [`b7f6e5f02`](npm/cli@b7f6e5f) Infer ownership of shrinkwrap files ([@isaacs](https://github.com/isaacs)) * [`54b095d77`](npm/cli@54b095d) [nodejs#235](npm/cli#235) Add spec to dist-tag remove function ([@theberbie](https://github.com/theberbie)) DEPENDENCIES * [`dc8f9e52f`](npm/cli@dc8f9e5) `pacote@9.5.7`: Infer the ownership of all unpacked files in `node_modules`, so that we never have user-owned files in root-owned folders, or root-owned files in user-owned folders. ([@isaacs](https://github.com/isaacs)) * [`bb33940c3`](npm/cli@bb33940) `cmd-shim@3.0.0`: * [`9c93ac3`](npm/cmd-shim@9c93ac3) [#2](npm/cmd-shim#2) [npm#3380](npm/npm#3380) Handle environment variables properly ([@basbossink](https://github.com/basbossink)) * [`2d277f8`](npm/cmd-shim@2d277f8) [nodejs#25](npm/cmd-shim#25) [nodejs#36](npm/cmd-shim#36) [nodejs#35](npm/cmd-shim#35) Fix 'no shebang' case by always providing `$basedir` in shell script ([@igorklopov](https://github.com/igorklopov)) * [`adaf20b`](npm/cmd-shim@adaf20b) [nodejs#26](npm/cmd-shim#26) Fix `$*` causing an error when arguments contain parentheses ([@satazor](https://github.com/satazor)) * [`49f0c13`](npm/cmd-shim@49f0c13) [nodejs#30](npm/cmd-shim#30) Fix paths for MSYS/MINGW bash ([@dscho](https://github.com/dscho)) * [`51a8af3`](npm/cmd-shim@51a8af3) [nodejs#34](npm/cmd-shim#34) Add proper support for PowerShell ([@ExE-Boss](https://github.com/ExE-Boss)) * [`4c37e04`](npm/cmd-shim@4c37e04) [#10](npm/cmd-shim#10) Work around quoted batch file names ([@isaacs](https://github.com/isaacs)) * [`a4e279544`](npm/cli@a4e2795) `npm-lifecycle@3.1.3` ([@isaacs](https://github.com/isaacs)): * fail properly if `uid-number` raises an error * [`7086a1809`](npm/cli@7086a18) `libcipm@4.0.3` ([@isaacs](https://github.com/isaacs)) * [`8845141f9`](npm/cli@8845141) `read-package-json@2.1.0` ([@isaacs](https://github.com/isaacs)) * [`51c028215`](npm/cli@51c0282) `bin-links@1.1.3` ([@isaacs](https://github.com/isaacs)) * [`534a5548c`](npm/cli@534a554) `read-cmd-shim@1.0.3` ([@isaacs](https://github.com/isaacs)) * [`3038f2fd5`](npm/cli@3038f2f) `gentle-fs@2.2.1` ([@isaacs](https://github.com/isaacs)) * [`a609a1648`](npm/cli@a609a16) `graceful-fs@4.2.2` ([@isaacs](https://github.com/isaacs)) * [`f0346f754`](npm/cli@f0346f7) `cacache@12.0.3` ([@isaacs](https://github.com/isaacs)) * [`ca9c615c8`](npm/cli@ca9c615) `npm-pick-manifest@3.0.0` ([@isaacs](https://github.com/isaacs)) * [`b417affbf`](npm/cli@b417aff) `pacote@9.5.8` ([@isaacs](https://github.com/isaacs)) TESTS * [`b6df0913c`](npm/cli@b6df091) [nodejs#228](npm/cli#228) Proper handing of /usr/bin/node lifecycle-path test ([@olivr70](https://github.com/olivr70)) * [`aaf98e88c`](npm/cli@aaf98e8) `npm-registry-mock@1.3.0` ([@isaacs](https://github.com/isaacs))
Full changelog: 6.11.1 (2019-08-20): Fix a regression for windows command shim syntax. * [`37db29647`](npm/cli@37db296) `cmd-shim@3.0.2` ([@isaacs](https://github.com/isaacs)) v6.11.0 (2019-08-20): A few meaty bugfixes, and introducing `peerDependenciesMeta`. FEATURES * [`a12341088`](npm/cli@a123410) [nodejs#224](npm/cli#224) Implements peerDependenciesMeta ([@arcanis](https://github.com/arcanis)) * [`2f3b79bba`](npm/cli@2f3b79b) [nodejs#234](npm/cli#234) add new forbidden 403 error code ([@claudiahdz](https://github.com/claudiahdz)) BUGFIXES * [`24acc9fc8`](npm/cli@24acc9f) and [`45772af0d`](npm/cli@45772af) [nodejs#217](npm/cli#217) [npm.community#8863](https://npm.community/t/installing-the-same-module-under-multiple-relative-paths-fails-on-linux/8863) [npm.community#9327](https://npm.community/t/reinstall-breaks-after-npm-update-to-6-10-2/9327,) do not descend into directory deps' child modules, fix shrinkwrap files that inappropriately list child nodes of symlink packages ([@isaacs](https://github.com/isaacs) and [@salomvary](https://github.com/salomvary)) * [`50cfe113d`](npm/cli@50cfe11) [nodejs#229](npm/cli#229) fixed typo in semver doc ([@gall0ws](https://github.com/gall0ws)) * [`e8fb2a1bd`](npm/cli@e8fb2a1) [nodejs#231](npm/cli#231) Fix spelling mistakes in CHANGELOG-3.md ([@XhmikosR](https://github.com/XhmikosR)) * [`769d2e057`](npm/cli@769d2e0) [npm/uid-number#7](npm/uid-number#7) Better error on invalid `--user`/`--group` configs. This addresses the issue when people fail to install binary packages on Docker and other environments where there is no 'nobody' user. ([@isaacs](https://github.com/isaacs)) * [`8b43c9624`](npm/cli@8b43c96) [nodejs#28987](nodejs#28987) [npm.community#6032](https://npm.community/t/npm-ci-doesnt-respect-npmrc-variables/6032) [npm.community#6658](https://npm.community/t/npm-ci-doesnt-fill-anymore-the-process-env-npm-config-cache-variable-on-post-install-scripts/6658) [npm.community#6069](https://npm.community/t/npm-ci-does-not-compile-native-dependencies-according-to-npmrc-configuration/6069) [npm.community#9323](https://npm.community/t/npm-6-9-x-not-passing-environment-to-node-gyp-regression-from-6-4-x/9323/2) Fix the regression where random config values in a .npmrc file are not passed to lifecycle scripts, breaking build processes which rely on them. ([@isaacs](https://github.com/isaacs)) * [`8b85eaa47`](npm/cli@8b85eaa) save files with inferred ownership rather than relying on `SUDO_UID` and `SUDO_GID`. ([@isaacs](https://github.com/isaacs)) * [`b7f6e5f02`](npm/cli@b7f6e5f) Infer ownership of shrinkwrap files ([@isaacs](https://github.com/isaacs)) * [`54b095d77`](npm/cli@54b095d) [nodejs#235](npm/cli#235) Add spec to dist-tag remove function ([@theberbie](https://github.com/theberbie)) DEPENDENCIES * [`dc8f9e52f`](npm/cli@dc8f9e5) `pacote@9.5.7`: Infer the ownership of all unpacked files in `node_modules`, so that we never have user-owned files in root-owned folders, or root-owned files in user-owned folders. ([@isaacs](https://github.com/isaacs)) * [`bb33940c3`](npm/cli@bb33940) `cmd-shim@3.0.0`: * [`9c93ac3`](npm/cmd-shim@9c93ac3) [#2](npm/cmd-shim#2) [npm#3380](npm/npm#3380) Handle environment variables properly ([@basbossink](https://github.com/basbossink)) * [`2d277f8`](npm/cmd-shim@2d277f8) [nodejs#25](npm/cmd-shim#25) [nodejs#36](npm/cmd-shim#36) [nodejs#35](npm/cmd-shim#35) Fix 'no shebang' case by always providing `$basedir` in shell script ([@igorklopov](https://github.com/igorklopov)) * [`adaf20b`](npm/cmd-shim@adaf20b) [nodejs#26](npm/cmd-shim#26) Fix `$*` causing an error when arguments contain parentheses ([@satazor](https://github.com/satazor)) * [`49f0c13`](npm/cmd-shim@49f0c13) [nodejs#30](npm/cmd-shim#30) Fix paths for MSYS/MINGW bash ([@dscho](https://github.com/dscho)) * [`51a8af3`](npm/cmd-shim@51a8af3) [nodejs#34](npm/cmd-shim#34) Add proper support for PowerShell ([@ExE-Boss](https://github.com/ExE-Boss)) * [`4c37e04`](npm/cmd-shim@4c37e04) [#10](npm/cmd-shim#10) Work around quoted batch file names ([@isaacs](https://github.com/isaacs)) * [`a4e279544`](npm/cli@a4e2795) `npm-lifecycle@3.1.3` ([@isaacs](https://github.com/isaacs)): * fail properly if `uid-number` raises an error * [`7086a1809`](npm/cli@7086a18) `libcipm@4.0.3` ([@isaacs](https://github.com/isaacs)) * [`8845141f9`](npm/cli@8845141) `read-package-json@2.1.0` ([@isaacs](https://github.com/isaacs)) * [`51c028215`](npm/cli@51c0282) `bin-links@1.1.3` ([@isaacs](https://github.com/isaacs)) * [`534a5548c`](npm/cli@534a554) `read-cmd-shim@1.0.3` ([@isaacs](https://github.com/isaacs)) * [`3038f2fd5`](npm/cli@3038f2f) `gentle-fs@2.2.1` ([@isaacs](https://github.com/isaacs)) * [`a609a1648`](npm/cli@a609a16) `graceful-fs@4.2.2` ([@isaacs](https://github.com/isaacs)) * [`f0346f754`](npm/cli@f0346f7) `cacache@12.0.3` ([@isaacs](https://github.com/isaacs)) * [`ca9c615c8`](npm/cli@ca9c615) `npm-pick-manifest@3.0.0` ([@isaacs](https://github.com/isaacs)) * [`b417affbf`](npm/cli@b417aff) `pacote@9.5.8` ([@isaacs](https://github.com/isaacs)) TESTS * [`b6df0913c`](npm/cli@b6df091) [nodejs#228](npm/cli#228) Proper handing of /usr/bin/node lifecycle-path test ([@olivr70](https://github.com/olivr70)) * [`aaf98e88c`](npm/cli@aaf98e8) `npm-registry-mock@1.3.0` ([@isaacs](https://github.com/isaacs))
No description provided.