Skip to content
This repository

Domains dispose problem #3559

Closed
wants to merge 2 commits into from

4 participants

Felix Geisendörfer Isaac Z. Schlueter tanguylebarzic Nodejs Jenkins
Felix Geisendörfer
Collaborator

Problem: When creating streams inside of a domain, Domain#dispose() will
not clean them up.

This patch demonstrates this problem using a test case, and provides a
possible fix by also adding implicit domain members to the domain.

Considering that I don't understand the design decisions that went into
only tracking explicit members at this point, this patch should be
considered as a request for comment on the current behavior / to start a
discussion on improving it.

Felix Geisendörfer RFC: Track implicit domain members
Problem: When creating streams inside of a domain, Domain#dispose() will
not clean them up.

This patch demonstrates this problem using a test case, and provides a
possible fix by also adding implicit domain members to the domain.

Considering that I don't understand the design decisions that went into
only tracking explicit members at this point, this patch should be
considered as a request for comment on the current behavior / to start a
discussion on improving it.
16cbec0
Isaac Z. Schlueter
Collaborator
isaacs commented June 27, 2012

In developing domains, I found that adding implicit members was a pretty significant performance hit. Can you try running bash benchmark/http.sh with and without this change, to see if it causes problems? We're on a new V8 now, so it might be a bit less hazardous.

Isaac Z. Schlueter
Collaborator
isaacs commented June 27, 2012

Sorry, that won't do anything. Use this command:

NODE_USE_DOMAINS=1 bash benchmark/http.sh

But, running without domains should definitely not cause a performance impact, either :)

Felix Geisendörfer
Collaborator

Did some benchmarks:

Not using domains:

$ bash benchmark/http.sh 
net.inet.ip.portrange.first: 12000 -> 12000
net.inet.tcp.msl: 1000 -> 1000
kern.maxfiles: 1000000 -> 1000000
kern.maxfilesperproc: 1000000 -> 1000000
pid 79936
Listening at http://127.0.0.1:8000/
create storedBytes[n]
Completed 3000 requests
Completed 6000 requests
Completed 9000 requests
Completed 12000 requests
Completed 15000 requests
Completed 18000 requests
Completed 21000 requests
Completed 24000 requests
Completed 27000 requests
Completed 30000 requests
Finished 30000 requests
Requests per second:    3608.00 [#/sec] (mean)

Domains as in 0.8.0:

$ NODE_USE_DOMAINS=1 bash benchmark/http.sh 
net.inet.ip.portrange.first: 12000 -> 12000
net.inet.tcp.msl: 1000 -> 1000
kern.maxfiles: 1000000 -> 1000000
kern.maxfilesperproc: 1000000 -> 1000000
pid 80170
Listening at http://127.0.0.1:8000/
create storedBytes[n]
Completed 3000 requests
Completed 6000 requests
Completed 9000 requests
Completed 12000 requests
Completed 15000 requests
Completed 18000 requests
Completed 21000 requests
Completed 24000 requests
Completed 27000 requests
Completed 30000 requests
Finished 30000 requests
Requests per second:    3496.04 [#/sec] (mean)

Domains as in 0.8.0 + implicit domain member tracking patch:

$ NODE_USE_DOMAINS=1 bash benchmark/http.sh 
net.inet.ip.portrange.first: 12000 -> 12000
net.inet.tcp.msl: 1000 -> 1000
kern.maxfiles: 1000000 -> 1000000
kern.maxfilesperproc: 1000000 -> 1000000
pid 79979
Listening at http://127.0.0.1:8000/
create storedBytes[n]
Completed 3000 requests
Completed 6000 requests
Completed 9000 requests
Completed 12000 requests
Completed 15000 requests
Completed 18000 requests
Completed 21000 requests
Completed 24000 requests
Completed 27000 requests
Completed 30000 requests
Finished 30000 requests
Requests per second:    2761.73 [#/sec] (mean)

You're right: There is a significant performance penalty for tracking domain members implicitly. But IMO domains are useless without it. I mean it's cool that I can match exceptions up to my http requests with them, but without being able to clean up, my processes will leak resources.

So even with the performance implications, I think domains should track implicit members by default. If there is a use case for domains without explicit member tracking (I don't think there is), we could easily make the behavior configurable on a per-domain basis.

Let me know what you think, and I'll finalize this patch to also take care of timers.

--fg

Felix Geisendörfer Improve domain usage in http_simple benchmark
There were two problems with the way domains were used in the benchmark:

a) A global domain was created. IMO this is not how one would normally
use domains, as an error caught by a global domain would be just as
difficult to handle as an 'uncaughtException' one. More importantly:
Setting up a global domain makes implicit domain member tracking
expensive as per-request domains will now also be tracked as members of
the global domain.

b) The per-request domains created in the benchmark were never entered.
While it didn't have an impact on the performance of the benchmark, it
does seem a bit pointless to not enter the domains, so this patch causes
them to be entered.
44a0f74
Felix Geisendörfer
Collaborator

@isaacs Please have a look at my commit above. IMO the benchmark used to determine the impact of domains was flawed, and after fixing it, my patch for adding implicit member tracking no longer causes any performance problems:

Fixed benchmark + without implicit member tracking patch:

Requests per second:    3332.68 [#/sec] (mean)

Fixed benchmark + implicit member tracking patch:

Requests per second:    3376.45 [#/sec] (mean)

So ... if you agree with my adjustment to the benchmark, what do you think about enabling implicit member tracking by default now? IMO it would make domains much more useful / easier to explain.

Let me know and I'll come up with a final set of patches / tests for this.

--fg

Isaac Z. Schlueter
Collaborator
isaacs commented June 29, 2012

@felixge That's fascinating. Let's discuss this in more detail after (or at) nodeconf.

The dom.enter() should be mostly unnecessary, but also harmless. The request and response objects are bound to it, so they'll enter during their events. However, it's probably the server's connection event entering the global domain that is the bottleneck, since it's added implicitly to the gdom domain.

I think it's clear that we need a better understanding of exactly where it's spending its time. Removing the explicit/implicit disparity would be nice, but we do have to be very careful we don't create performance regression landmines.

Felix Geisendörfer
Collaborator

@isaacs the main thing 44a0f74 was fixing was essentially a leak in the benchmark. If you do implicit member tracking, and you have a global domain + domains for each request, the domains for each request will all be added as children to the global domain, and never cleaned up.

I remember our discussion at nodeconf where you said the global domain was useful in your scenario (where you'd shut down the process after any domain errors), but IMO the biggest use case I see for domains is keeping long-running connections alive. This means the process cannot be shutdown (to clean up leaks).

So from your perspective: What would you like to see to move this forward? You mentioned a more detailed analysis on where time is spend? What about making the implicit member tracking optional, this way you can ignore the performance issues for your use case (having a global domain), and I can get good performance on my use case (not having a global domain) at the same time.

tanguylebarzic

Hi,

Any news on this? I have a similar issue:
See gist https://gist.github.com/4332159.
Launching the server, then calling http://localhost:1337/ works the five first times, but after that calls to curl just hang.

From what I saw, it's because the underlying socket of the http.request is not destroyed, resulting in a starvation of the sockets pool. Calling newDomain.dispose inside process.nextTick 'fixes' the problem. From what I was able to understand, it's because domain._disposed is set to true later, allowing some EventEmitter to enter the domain and get cleaned better.

Would be great to be able to use domains without worries :).

Nodejs Jenkins
Collaborator

Can one of the admins verify this patch?

Isaac Z. Schlueter
Collaborator

This patch is no longer relevant. @trevnorris is in the process of significantly refactoring how domains work in master, and domain.dispose() has been widely acknowledged as a failure of an abstraction (it doesn't do what it claims, and is unnecessary and unsafe.)

Isaac Z. Schlueter isaacs closed this August 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Jun 27, 2012
Felix Geisendörfer RFC: Track implicit domain members
Problem: When creating streams inside of a domain, Domain#dispose() will
not clean them up.

This patch demonstrates this problem using a test case, and provides a
possible fix by also adding implicit domain members to the domain.

Considering that I don't understand the design decisions that went into
only tracking explicit members at this point, this patch should be
considered as a request for comment on the current behavior / to start a
discussion on improving it.
16cbec0
Jun 29, 2012
Felix Geisendörfer Improve domain usage in http_simple benchmark
There were two problems with the way domains were used in the benchmark:

a) A global domain was created. IMO this is not how one would normally
use domains, as an error caught by a global domain would be just as
difficult to handle as an 'uncaughtException' one. More importantly:
Setting up a global domain makes implicit domain member tracking
expensive as per-request domains will now also be tracked as members of
the global domain.

b) The per-request domains created in the benchmark were never entered.
While it didn't have an impact on the performance of the benchmark, it
does seem a bit pointless to not enter the domains, so this patch causes
them to be entered.
44a0f74
This page is out of date. Refresh to see the latest.
8  benchmark/http_simple.js
@@ -13,15 +13,8 @@ var fixed = makeString(20 * 1024, 'C'),
13 13
 
14 14
 var useDomains = process.env.NODE_USE_DOMAINS;
15 15
 
16  
-// set up one global domain.
17 16
 if (useDomains) {
18 17
   var domain = require('domain');
19  
-  var gdom = domain.create();
20  
-  gdom.on('error', function(er) {
21  
-    console.log('Error on global domain', er);
22  
-    throw er;
23  
-  });
24  
-  gdom.enter();
25 18
 }
26 19
 
27 20
 var server = http.createServer(function (req, res) {
@@ -29,6 +22,7 @@ var server = http.createServer(function (req, res) {
29 22
     var dom = domain.create();
30 23
     dom.add(req);
31 24
     dom.add(res);
  25
+    dom.enter();
32 26
   }
33 27
 
34 28
   var commands = req.url.split('/');
1  lib/events.js
@@ -28,6 +28,7 @@ function EventEmitter() {
28 28
     domain = domain || require('domain');
29 29
     if (domain.active && !(this instanceof domain.Domain)) {
30 30
       this.domain = domain.active;
  31
+      this.domain.members.push(this);
31 32
     }
32 33
   }
33 34
 }
69  test/simple/test-domain-run-bug.js
... ...
@@ -0,0 +1,69 @@
  1
+// Copyright Joyent, Inc. and other Node contributors.
  2
+//
  3
+// Permission is hereby granted, free of charge, to any person obtaining a
  4
+// copy of this software and associated documentation files (the
  5
+// "Software"), to deal in the Software without restriction, including
  6
+// without limitation the rights to use, copy, modify, merge, publish,
  7
+// distribute, sublicense, and/or sell copies of the Software, and to permit
  8
+// persons to whom the Software is furnished to do so, subject to the
  9
+// following conditions:
  10
+//
  11
+// The above copyright notice and this permission notice shall be included
  12
+// in all copies or substantial portions of the Software.
  13
+//
  14
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
  15
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
  16
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
  17
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
  18
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
  19
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
  20
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
  21
+
  22
+var assert = require('assert');
  23
+var common = require('../common.js');
  24
+var net = require('net');
  25
+var domain = require('domain');
  26
+
  27
+console.log('starting test ...');
  28
+
  29
+var connections =0;
  30
+var server = net.createServer(function(socket) {
  31
+  connections++;
  32
+
  33
+  socket.on('end', function() {
  34
+    connections--;
  35
+
  36
+    if (connections === 0) {
  37
+      server.close();
  38
+    }
  39
+  });
  40
+
  41
+  socket.write('hi');
  42
+});
  43
+
  44
+server.listen(common.PORT, function() {
  45
+  var d = domain.create();
  46
+
  47
+  d.on('error', function(err) {
  48
+    assert.equal(d.members.length, 1);
  49
+
  50
+    console.log('caught domain exception: %s', err.message);
  51
+    d.dispose();
  52
+  });
  53
+
  54
+
  55
+  d.run(function() {
  56
+    var socket = net.createConnection(common.PORT);
  57
+
  58
+    socket.setEncoding('utf-8');
  59
+
  60
+    socket.on('data', function(data) {
  61
+      console.log('received data from server: %s', data);
  62
+      throw new Error('Boom!');
  63
+    });
  64
+  });
  65
+});
  66
+
  67
+process.on('exit', function() {
  68
+  assert.equal(connections, 0);
  69
+});
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.