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

worker_threads: worker handles not cleaned up in async_hooks API #26535

Closed
chjj opened this issue Mar 8, 2019 · 2 comments
Closed

worker_threads: worker handles not cleaned up in async_hooks API #26535

chjj opened this issue Mar 8, 2019 · 2 comments
Labels
worker Issues and PRs related to Worker support.

Comments

@chjj
Copy link
Contributor

chjj commented Mar 8, 2019

  • Version: v11.10.1
  • Platform: Linux 4.20.11-arch1-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Wed Feb 20 21:11:12 UTC 2019 x86_64 GNU/Linux
  • Subsystem: worker_threads

Not sure if it's a bug or not, but I've noticed that destroy is never executed for the asyncIds pertaining to workers.

'use strict';

const threads = require('worker_threads');

if (!threads.isMainThread) {
  console.log('Thread: ' + threads.threadId);
  return;
}

const hooks = require('async_hooks');

async function spawn(file) {
  const worker = new threads.Worker(file);
  return new Promise((resolve, reject) => {
    worker.on('exit', resolve);
  });
}

async function wait(ms) {
  return new Promise(cb => setTimeout(cb, ms));
}

(async () => {
  const active = new Map();

  const hook = hooks.createHook({
    init(id, type) {
      if (type === 'WORKER')
        active.set(id, type);
    },
    destroy(id) {
      active.delete(id);
    }
  });

  hook.enable();

  for (let i = 0; i < 10; i++)
    await spawn(__filename);

  await wait(1000);

  hook.disable();

  await wait(1000);

  console.log(active);
})();

Output:

Thread: 1
Thread: 2
Thread: 3
Thread: 4
Thread: 5
Thread: 6
Thread: 7
Thread: 8
Thread: 9
Thread: 10
Map {
  3 => 'WORKER',
  23 => 'WORKER',
  39 => 'WORKER',
  55 => 'WORKER',
  71 => 'WORKER',
  87 => 'WORKER',
  103 => 'WORKER',
  119 => 'WORKER',
  135 => 'WORKER',
  151 => 'WORKER' }
@addaleax addaleax added the worker Issues and PRs related to Worker support. label Mar 9, 2019
addaleax added a commit to addaleax/node that referenced this issue Mar 9, 2019
Destroy the `Worker` class earlier, because we don’t need access
to it once the thread has stopped and all resources have been
cleaned up.

Fixes: nodejs#26535
@addaleax
Copy link
Member

addaleax commented Mar 9, 2019

I don’t think it’s quite a bug, because it’s only happening because the destroy() hook for Workers currently relies on garbage collection (like for many other objects); the Worker object itself owns no resources anymore when it is collected.

However, this is still fixable in the sense that we can destroy the objects earlier, if we want: #26542

@chjj
Copy link
Contributor Author

chjj commented Mar 9, 2019

Ahh, I see. No worries then. I was just concerned the handles were not being cleaned up or something.

@danbev danbev closed this as completed in c51735f Mar 12, 2019
BridgeAR pushed a commit that referenced this issue Mar 13, 2019
Destroy the `Worker` class earlier, because we don’t need access
to it once the thread has stopped and all resources have been
cleaned up.

PR-URL: #26542
Fixes: #26535
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR pushed a commit that referenced this issue Mar 14, 2019
Destroy the `Worker` class earlier, because we don’t need access
to it once the thread has stopped and all resources have been
cleaned up.

PR-URL: #26542
Fixes: #26535
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants