Skip to content
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

cluster.worker.id not present when using preload module (-r) #1269

Closed
megastef opened this issue Mar 26, 2015 · 8 comments
Closed

cluster.worker.id not present when using preload module (-r) #1269

megastef opened this issue Mar 26, 2015 · 8 comments
Labels
cluster Issues and PRs related to the cluster subsystem.

Comments

@megastef
Copy link

I'm using for logging for each forked worker the property worker.id - this works fine for node 0.10,0.12 and io.js up to 1.5 - in 1.6.2, only when I use the new -r / preload option to preload the logging module I get following error:

  postfix = '.worker-' + cluster.worker.id 
                                       ^
  TypeError: Cannot read property 'id' of undefined

How can this happen?

The related source code:

if (!cluster.isMaster)
    postfix = '.worker-' + cluster.worker.id 

https://nodejs.org/api/cluster.html#cluster_cluster_worker
https://nodejs.org/api/cluster.html#cluster_worker_id

@silverwind silverwind added the cluster Issues and PRs related to the cluster subsystem. label Mar 26, 2015
@meandmycode
Copy link

Possibly ping @ofrobots

@ofrobots
Copy link
Contributor

@megastef Do you have a reduced test-case I can try? I just tried this: https://gist.github.com/ofrobots/81a76e326a293ab568b4 but that doesn't reproduce the issue.

@megastef
Copy link
Author

Ok, thanks - I will try to reproduce it - and create test case (need to check if was on ARM or x86 as well).

@megastef
Copy link
Author

@ofrobots Added a comment to your gist to reproduce it.

@megastef
Copy link
Author

BTW: This is the real use case. I load our monitoring agent via -r - normally it would need to modify source code to load it with "var spm = require('spm-agent-nodejs')"
The result when I use -r is here: https://gist.github.com/megastef/0dd384e09ec0465a4fcb

The idea was to write no own wrapper/proxy script by using -r option. It would allow to load monitoring/instrumentation functions without modification of original source-code.

@ofrobots
Copy link
Contributor

The problem was due to the fact that the preload modules were getting executed before the code in node.js that is supposed to call cluster._setupWorker. The following patch fixes the problem.

diff --git a/src/node.js b/src/node.js
index 6346529..095b4c6 100644
--- a/src/node.js
+++ b/src/node.js
@@ -71,6 +71,17 @@
     } else {
       // There is user code to be run

+      // If this is a worker in cluster mode, start up the communication
+      // channel. This needs to be done before any user code gets executed
+      // (including preload modules)
+      if (process.argv[1] && process.env.NODE_UNIQUE_ID) {
+        var cluster = NativeModule.require('cluster');
+        cluster._setupWorker();
+
+        // Make sure it's not accidentally inherited by child processes.
+        delete process.env.NODE_UNIQUE_ID;
+      }
+
       // Load any preload modules
       if (process._preload_modules) {
         var Module = NativeModule.require('module');
@@ -87,16 +98,6 @@
         var path = NativeModule.require('path');
         process.argv[1] = path.resolve(process.argv[1]);

-        // If this is a worker in cluster mode, start up the communication
-        // channel.
-        if (process.env.NODE_UNIQUE_ID) {
-          var cluster = NativeModule.require('cluster');
-          cluster._setupWorker();
-
-          // Make sure it's not accidentally inherited by child processes.
-          delete process.env.NODE_UNIQUE_ID;
-        }
-
         var Module = NativeModule.require('module');

         if (global.v8debug &&

I will submit a PR. Thanks for reporting!

@megastef
Copy link
Author

1000 thx for fixing :)

ofrobots added a commit to ofrobots/node that referenced this issue Apr 1, 2015
We need to process cluster workers before any preload modules is
executed. Otherwise, the child processes are not correctly disovered
as clustered workers inside the preloaded modules.

Fixes: nodejs#1269
bnoordhuis pushed a commit that referenced this issue Apr 3, 2015
We need to process cluster workers before any preload modules is
executed. Otherwise, the child processes are not correctly disovered
as clustered workers inside the preloaded modules.

Fixes: #1269
PR-URL: #1314
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@bnoordhuis
Copy link
Member

Fixed by b6e22c4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants