Domains support

[sidorares]

This change is very similar to sidorares/node-mysql2#75

Usually connection/pool is created in top level domain (or no domain at all) and you would expect query(cb) callback to be called in the context of query() domain (often http request level domain). Instead, connection EventEmitter captures connect time domain and it is used later in all callbacks. This PR wraps all api callbacks with bindToCurrentDomain function. There should be no performance impact when domains are not used. See test and sidorares/node-mysql2#73 for code examples

[felixge]

LGTM. I've never really worked with domains as they don't solve the problem of callback safety, but I'm all for it if people find them useful.

[sidorares]

@dresende @dougwilson - wdyt?

[dougwilson]

It looks good to me. Do we think this should also be added to the callbacks in Pool?

[sidorares]

You are right. For some reason I was thinking that this is unnecessary. because already happens in underlying query call. I'll add this to pool callbacks

[dougwilson]

@sidorares yea, I thought so too. It may, but at the very least Pool.prototype.query won't, since the query is wrapped in a call to Pool.prototype.getConnection which will loose the domain. This would also loose the domain for users wrapping their query in this, so we probably should :)

[sidorares]

@dougwilson - added test, but can't make it to fail, pooled connection seems to use correct domain.

[dougwilson]

I think to see the pool fail, you have to have an idle connection in the pool, then calling getConnection will loose the domain. If getConnection actually creates a brand new connection then the domain stuff is covered by Connection methods.

---

From: Andrey Sidorovmailto:[email protected]
Sent: ‎1/‎16/‎2014 17:44
To: felixge/node-mysqlmailto:[email protected]
Cc: Douglas Christopher Wilsonmailto:[email protected]
Subject: Re: [node-mysql] Domains support (#709)

@dougwilson - added test, but can't make it to fail, pooled connection seems to use correct domain.

---

Reply to this email directly or view it on GitHub:
#709 (comment)

[sidorares]

still not failing...

  // make idle connection first
  pool.getConnection(function(err, conn) {
    conn.release();
    d3.run(function() {
      pool.query('SELECT 2', function(err, _rows, _fields) {
        if (err) throw err;
        throw new Error('inside domain 3');
      });
    });
  });

[dougwilson]

Yea, sorry, after I sent that I realized I said it wrong. I actually meant the domain will vanish when getConnection has to wait for a connection to become available. I'll make a test, haha. I can't articulate well right now it seems :)

[dougwilson]

See if this fails, plz :)

var pool = common.createPool({connectionLimit: 1});

pool.getConnection(function(err, conn) {
  d3.run(function() {
    pool.query('SELECT 2', function(err, _rows, _fields) {
      if (err) throw err;
      throw new Error('inside domain 3');
    });
  });
  process.nextTick(function () {
    conn.release();
  });
});

[dougwilson]

If the above does fail, I think it would fix it by binding the domain to cb at https://github.com/felixge/node-mysql/blob/master/lib/Pool.js#L66

If it does not fail, then I guess we won't worry about it unless I can come with with something that does fail :)

[sidorares]

Finally - test fails without pool.js change and passes with. @dougwilson - could you please review the test?

[dougwilson]

Sweet. I think we are good to go. I can't think of any other holes there would be.

[sidorares]

thanks for help @dougwilson - very much appreciated! merging...

[felixge]

💖 thx guys

[sidorares]

see also: #308 #489