Add 'mongo_client' argument to AgnosticClient

[tjensen]

This change adds a new mongo_client argument when creating a new MotorClient or AsyncIOMotorClient instance, setting the delegate object to a pre-existing MongoClient instead of creating a new instance.

Background

I help maintain a project that uses Motor and has a very large number of tests, the majority of which involve MongoDB accesses. These tests use Tornado's AsyncTestCase and create new MotorClient instances in their setUp methods. As the tests run, they become progressively slower and, especially on weaker computers, eventually hang. Attaching a debugger to the hung Python process shows that there are a large number of active threads, the number roughly corresponding to the number of MotorClient instances that were created, thus far. These threads appear to be running the PyMongo _process_periodic_tasks code run through the periodic executor.

To avoid the hanging issue and improve overall test performance, we would like to be able to re-use the same MotorClient instance in all tests that need database access but, since MotorClient needs an IO loop and Tornado's AsyncTestCase creates a new IO loop for each test, that does not appear possible. A decent alternative is to re-use a single MongoClient instance for all tests and pass that MongoClient when creating a new MotorClient in the setUp method.

[ajdavis]

Neat. Thanks for explaining the problem, that definitely sounds plausible. But I don't think that the "motor_client" parameter is the right approach. It's a powerful new feature that anyone could use, not just Motor's test suite, and if people use it then they bypass some logic that Motor needs to do: see the AgnosticClient constructor for Motor's current logic for creating a MongoClient. We want to preserve that logic and preserve Motor's freedom to change it, without worrying about users who have bypassed it with the "motor_client" parameter.

I have another idea: MotorClient.close() calls MongoClient.close(), which signals the background thread(s) to terminate and then waits for them. Is it possible to call MotorClient.close() on every MotorClient the test suite creates, or at least on most of them, to prevent the accumulation of background threads?

Most of the MotorClients and AsyncIOMotorClients created in the suite come from MotorTest.motor_client(), MotorTest.motor_rsc(), and the equivalent AsyncIOTestCase methods. There, try something like:

client = motor.MotorClient( ... options ... )
self.addCleanup(client.close)
return client

That should handle most of the test suite's clients. I believe addCleanup is available on all Python versions Motor supports. Will a technique like that solve the problem?

[tjensen]

@ajdavis Unfortunately, calling close as part of the test cleanup does not solve the issue. The problem seems to be that the periodic executor thread does not terminate until the MongoClient instance that created it is garbage collected. Ensuring that the client gets garbage collected does not appear to be straightforward to achieve.

I created this example project to demonstrate the issue: https://github.com/tjensen/motor-test-example

The example project requires Python 3.6 and includes about 2000 tests that use Motor to talk to MongoDB. Per your suggestion, the test tearDown calls close to try to terminate the periodic executor threads.

To run the tests, run:

$ MONGODB_URI=mongodb://<your-database> nosetests

If you don't set MONGODB_URI, the tests will try to connect to a database called motor-test-example on localhost.

The tests hang pretty reliably on my MacBook. I can break in to the Python process with lldb to get a list of active threads:

$ lldb -p 5536
(lldb) process attach --pid 5636
Process 5636 stopped
...snip...
(lldb) thread list
Process 5636 stopped
* thread #1: tid = 0x5dfeb6, 0x00007fff64a32a46 libsystem_kernel.dylib`__psynch_mutexwait + 10, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  thread #2: tid = 0x5dfec1, 0x00007fff64a32a46 libsystem_kernel.dylib`__psynch_mutexwait + 10
  thread #3: tid = 0x5dfec2, 0x00007fff64a32a16 libsystem_kernel.dylib`__psynch_cvwait + 10
...snip...
  thread #1612: tid = 0x5e1d33, 0x00007fff64a32a46 libsystem_kernel.dylib`__psynch_mutexwait + 10
  thread #1613: tid = 0x5e1d34, 0x00007fff64a32a46 libsystem_kernel.dylib`__psynch_mutexwait + 10
  thread #1614: tid = 0x5e1d35, 0x00007fff64a32a16 libsystem_kernel.dylib`__psynch_cvwait + 10

As you can see, in this case there were 1614 threads running at the time of the hang.

Thank you for considering this PR. I understand the change I proposed may not align with the Motor's future direction, so any suggestions you can offer to solve this issue are appreciated.

[ShaneHarvey]

Jesse + Tim, I think pymongo could improve this situation by stopping the "pymongo_kill_cursors_thread" when a MongoClient is closed. Then the thread would be destroyed without the need for garbage collection. This also brings the kill cursors thread's lifetime more in line with the Topology's lifetime.

I tested this theory using https://github.com/tjensen/motor-test-example and this pymongo branch and threads peeked around ~30 and remained stable for the entire nosetests run. Before the change I also saw the number of threads climb to over 1300+ however I could not reproduce a test hang.

[ajdavis]

Shane thank you! Yeah, this sounds like a PyMongo bug: I'd expected MongoClient.close to stop and join all the background threads, not just the monitor threads.

[ajdavis]

(By the way, I notice that I had misunderstood the original bug report. I see this is about Tim's application test suite, not Motor's self-tests.)