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

[Feedback needed] fix; exit all manually entered NodeJS domains #367

Merged
merged 1 commit into from
Dec 16, 2017
Merged

[Feedback needed] fix; exit all manually entered NodeJS domains #367

merged 1 commit into from
Dec 16, 2017

Conversation

aheckmann
Copy link
Contributor

@aheckmann aheckmann commented Dec 14, 2017

I've found an issue with core-js Promises when used in Node 4x. Using the following script demonstrates that when domains are in use and a then() callback throws an error, the active domain is left in an entered state. This causes the domain to not be garbage collected, leading to memory leaks of not just the domain, but of anything added to the domain (a pretty big leak when using them to handle http server errors as outlined in the domain documentation).

'use strict';

var core = require('core-js/library');
var domain = require('domain');

var Type = {
  'core': core.Promise,
  'native': Promise
};

var type = process.argv[2]; // `node thisfile.js core` or `node thisfile.js native`
console.log('using %s', type);
var P = Type[type];

function reportDepth(state) {
  console.log(state + ', current domain depth is ' + domain._stack.length);
}

var resolver;
var d = domain.create();
// d.add(somethingLarge);

d.run(function() {
  reportDepth('In domain.run()');

  new P(function(resolve) {
    resolver = resolve;
  }).then(function() {
    reportDepth('Prior to throwing an error');

    throw new Error('Cause the domain to be left in an entered state');
  }).catch(function() {
    reportDepth('In exception handler');
  });
});

reportDepth('After domain exit');
resolver('asynchronous resolve()');

process.on('exit', function() {
  reportDepth('At exit');
});

This PR fixes the issue I'm facing but I'm not sure how / where to write tests for this project. I'm also not sure if I need to update both modules/es6.promise.js and library/modules/es6.promise.js. Please advise.

@zloirock
Copy link
Owner

LGTM

@zloirock zloirock merged commit 6f97920 into zloirock:master Dec 16, 2017
@aheckmann aheckmann deleted the always_close_opened_domains branch December 18, 2017 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants