pool - Corrects domain issue on _createConnection

[owenallenaz]

I'll start with, I hate domains, but while working on some unit tests we discovered an issue related to issuing queries in a rapid fire manner which caused our domains to go haywire. After a lot of tracing we discovered the root problem.

Right now, due to a pull request I made some time ago you guys added a setting for binding the query callback to the domain that it was originally initiated on. That works fine. The problem is inside pool.js there is a spot where you will dynamically grow the pool after queries have been initiated. This can cause a call to .find() to result in a new pool member. If the .find() was on a domain, then the new pool member is permanently bound to the domain. So all future queries from that pool member, whether or not they originate from a domain, will be bound to a domain, because it was created inside the internal callback structure of the .find().

Here's a psuedo-code example

setTimeout(function() {
	domain.run(function() {
		process.domain.id = "test!";
		// if this .find() triggers a growing of the pool via _createConnection, then the new pool member is permanently bound to the domain
		collection.find(someFilter).toArray(function() {
			domain.exit();
                        // do something
		})
	});
}, 5);

setTimeout(function() {
	console.log("process.domain.id", process.domain.id);
	// process.domain.id undefined
	collection.find(someFilter).toArray(function() {
		console.log("process.domain.id", process.domain.id);
		// process.domain.id "test!"
		// we've become bound to a previous domain because the pool connection that served us was created on a domain
                // even though this query was not issued on a domain, and it's callback should not be on a domain
	});
}, 15);

To me this makes sense because the purpose of domainsEnabled is to bind queries to a domain. So that way if I issue a query on a domain .find({}, cb), my callback is still on that domain. Now, if you guys want to be absolute sticklers, it could be refined so that we would check if the original Pool.connect was created with a domain in context, and if so we could allow THAT domain to remaind but others to not. I just don't know how realistic that use-case is, since it feels like an anti-pattern to me to create a connection on a domain, rather than issuing the connections via domain. If that's what you would like I can correct the pull request.

[mbroadst]

@owenallenaz I'm sorry this PR has gone unanswered for so long. We are going to completely refactor the connection pool this quarter, and will make sure proper support for domains is considered high priority. Thank you for the contribution.

[owenallenaz]

Thanks for the response. We've been working on refactoring our application to utilize Promises via async/await, because when that is properly done the Domain thing becomes a non-factor since domain is no longer needed to correctly handle deep errors within callbacks. To me, it's on the table in a pool refactor that you may not have to support domain at all, if you have properly implemented Promise semantics.

[bompus]

@mbroadst Have you had a chance to look at this yet, or is the planned connection pool refactor still pending completion?

[mbroadst]

@bompus we have a PR up for the proof of concept of a connection pool rewrite which is compliant with our newly written Connection Monitoring & Pooling spec. Unfortunately, that spec was written to provide a foundation across all MongoDB drivers on which to build more complicated behaviors, so things like priority queueing of messages (like your other PR for round-robin behavior) and domains are out of scope. It will probably be another minor release or two before we merge in the new pool code.