cluster: emit 'message' event on cluster master #861
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ping @bnoordhuis |
|
LGTM. +1 to consistency. |
| @@ -5,7 +5,7 @@ var net = require('net'); | |||
|
|
|||
| function forEach(obj, fn) { | |||
| Object.keys(obj).forEach(function(name, index) { | |||
| fn(obj[name], name, index); | |||
| fn(obj[name], name); | |||
Creates two new internal modules (child_process and socket_list) for better readability. Exposes the ChildProcess constructor from the child_process module so one can now `require(‘child_process’).ChildProcess` Fixes: nodejs#1751 PR-URL: nodejs#1760 Reviewed-By: Chris Dickinson <[email protected]>
This reverts commit 3c44100. Reverted for breaking node-heapdump[0]. AsyncWrap assigns a class id but does not set a v8::RetainedObjectInfo provider callback with v8::HeapProfiler::SetWrapperClassInfoProvider(). The result is a null pointer dereference when taking a heap snapshot. It can probably be solved by setting a generic provider callback inside the AsyncWrap constructor but that may have performance ramifications that need to be investigated first. I move to revert it for now. [0] https://github.com/bnoordhuis/node-heapdump PR-URL: nodejs#1827 Reviewed-By: Trevor Norris <[email protected]>
Add a regression test for nodejs#1827. PR-URL: nodejs#1828 Reviewed-By: Trevor Norris <[email protected]>
`flushHeaders` should work for header written with `writeHead`. PR-URL: nodejs#1695 Reviewed-By: Ben Noordhuis <[email protected]>
On a few of our installations (namely CentOS), passing 'INFO' resulted in a silent loglevel. Use a logging constant instead. Fixes: nodejs/build#104 PR-URL: nodejs#1842 Reviewed-By: Rod Vagg <[email protected]>
The Socket writable only change was added and implemented in the constructor around 5885f46, but this was never removed. The libev counter issue is no longer prudent; the test remains in test/sequential/test-regress-GH-1726. PR-URL: nodejs#1819 Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: nodejs#1829 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Every npm version bump requires a few patches to be floated on node-gyp for io.js compatibility. These patches are found in 03d1992, 5de334c, and da730c7. This commit squashes them into a single commit. PR-URL: nodejs#990 Reviewed-By: Ben Noordhuis <[email protected]>
The delay-load hook allows node.exe/iojs.exe to be renamed. See efadffe for more background. This commit is a combined squash of the following previous patches: ba93c58, 3bda6cb, 0d6d3dd. PR-URL: nodejs#1763 Reviewed-By: Jeremiah Senkpiel <[email protected]>
When the preload module is not a abs/relative path, we should use the standard search mechanism of looking into the node_modules folders outwards. The current working directory is deemed to be the 'requiring module', i.e. parent. The search path starts from cwd outwards. Fixes: nodejs#1803 PR-URL: nodejs#1812 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
This change eliminates an unnecessary setTimeout() in the test. PR-URL: nodejs#1821 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Brendan Ashworth <[email protected]>
PR-URL: nodejs#1808 Notable Changes: * node: Speed-up require() by replacing usage of fs.statSync() and fs.readFileSync() with internal variants that are faster for this use-case and do not create as many objects for the garbage collector to clean up. The primary two benefits are: significant increase in application start-up time on typical applications and better start-up time for the debugger by eliminating almost all of the thousands of exception events. (Ben Noordhuis) nodejs#1801. * node: Resolution of pre-load modules (-r or --require) now follows the standard require() rules rather than just resolving paths, so you can now pre-load modules in node_modules. (Ali Ijaz Sheikh) nodejs#1812. * npm: Upgraded npm to v2.11.0. New hooks for preversion, version, and postversion lifecycle events, some SPDX-related license changes and license file inclusions. See the release notes for full details.
The improper deprecation of the property broke a feature in the request module used by the bundled npm. This reverts the deprecation part of this change. PR-URL: nodejs#1852 Fixes: nodejs#1850 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
While checking the return values from icu-i18n, we didn't validate the content before passing it to the build system. Also make cflags parsing more robust by avoiding empty strings. Fixes: nodejs#1787 PR-URL: nodejs#1789 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: nodejs#1856 Notable Changes: * http: reverts the removal of an undocumented `client` property on client connections, this property is being used in the wild, most notably by https://github.com/request/request which is used by npm. (Michaël Zasso) [nodejs#1852](nodejs#1852).
This fixes a platform inconsistency between BSD and GNU `cp` where `deps/npm` would be copied into a subdirectory of `test-npm` on Linux, but not on OS X. PR-URL: nodejs#1853 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
PR-URL: nodejs#1859 Reviewed-By: Brendan Ashworth <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Options have been moved into the NodeOptions class. A new global, node_options now exists and is used to access the options after the command line arguments have been parsed. PR-URL: nodejs#1804 Reviewed-By: Ben Noordhuis <[email protected]>
This reverts commit c0e7bf2. There are a few edge cases that can cause a crash and need to be properly handled. PR-URL: nodejs#1862 Reviewed-By: Ben Noordhuis <[email protected]>
- `sh.css` already exists in `api_assets`
- `sh_vim-dark.css` is unused, but used in the repo `node-website`
now
Reviewed-by: Trevor Norris <[email protected]>
Signed-off-by: Julien Gilli <[email protected]>
PORT-FROM: joyent/node @ 0c50195
PR-URL: nodejs#1770
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Clarify that synchronous functions in fs with no return value return undefined. Specify that fs.openSync() returns an integer and fs.existsSync() returns true or false. Fixes: nodejs/node-v0.x-archive#9313 PR: nodejs/node-v0.x-archive#9359 Reviewed-By: Julien Gilli <[email protected]> PORT-FROM: joyent/node @ 51fe319 PR-URL: nodejs#1770 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Conflicts: doc/api/fs.markdown
PR-URL: nodejs#1858 Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#2189
As per the dicussion in nodejs#569, this patch issues a deprecation warning when freelist module is required. A test file for freelist is also added. PR-URL: nodejs#2176 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Brendan Ashworth <[email protected]>
sam-github
force-pushed
the
cluster-event-consistency
branch
from
b1606d8
to
d539f73
Compare
Refer: nodejs#2102 PR-URL: nodejs#2199 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Ref: nodejs#2102 PR-URL: nodejs#2200 Reviewed-By: Jeremiah Senkpiel <[email protected]>
|
Had to rebase onto master so I could run CI on this: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/166/ |
|
@sam-github Are you sure that the rebasing is fine? |
Changes to core modules do not take effect unless recompiled. Tip new contributors about this when describing how to run tests in contribution guide. Removed `jslint` from first test command example, as jslint is included when running `make test`. Fixed wrong path of example stream2-transform test. PR-URL: nodejs#2051 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: nodejs#2191 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
This patch makes the skip messages consistent so that the TAP plugin
in CI can parse the messages properly. The format will be
1..0 # Skipped: [Actual reason why the test is skipped]
PR-URL: nodejs#2109
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
This patch uses `return` statement to skip the test instead of using `process.exit` call. PR-URL: nodejs#2109 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
For consistency with the worker 'exit', 'online', 'disconnect', and 'listening' events which are emitted on worker and cluster, also emit 'message' on cluster.
sam-github
force-pushed
the
cluster-event-consistency
branch
from
d539f73
to
766b612
Compare
|
@thefourtheye looks OK to me: master...sam-github:cluster-event-consistency |
|
Still looking at the test failures.... |
|
CI is happy |
|
This PR has 7760 changed files :O |
|
OK, all the linux passed, except for one that is hung: https://jenkins-iojs.nodesource.com/job/iojs+pr+linux/169/ windows is green, except for the build that failed with a git error os x passed https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/186/?auto_refresh=true I'll land this on master. @thefourtheye this PR is just |
sam-github
added
the
semver-minor
sam-github
added a commit
to sam-github/node
that referenced
this pull request
Jul 23, 2015
For consistency with the worker 'exit', 'online', 'disconnect', and 'listening' events which are emitted on worker and cluster, also emit 'message' on cluster. Reviewed-by: Sam Roberts <[email protected]> Reviewed-by: Christian Tellnes <[email protected]> Reviewed-by: Stephen Belanger <[email protected]> PR-URL: nodejs#861
|
Landed in 66fc8ca |
This was referenced Sep 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Four of the following five events that are emitted on workers are also emitted on the cluster master.
Which one is not?
The node API should not lend itself to trivia quiz questions.
Full disclosure: This change has been rejected before on the basis that the cluster module doesn't need more "sugar", but I'd like it to be reconsidered, not from a point of view of whether it needs more sugar (it doesn't), but from a point of view of whether its reasonable to have such inconsistent API choices.
/to @bnoordhuis