Node.js driver 3.2.x leaking timers running in Node.js v12

Wilco_Waaijer · February 25, 2020, 5:48pm

We where having some memory filling up and some base line CPU usage increase in our Node.js applications. The cause seems to be the MongoB driver. After some investigation we found out that the “mongodb-core”-package (being used in mongodb v3.0.x - v3.2.x) is leaking timers due to changes in timers for Node.js v12.

The MongoDB Node.js driver versions 3.0.x till 3.2.x are reported to support Node.js v12.x.x here: https://docs.mongodb.com/ecosystem/drivers/driver-compatibility-reference/#reference-compatibility-language-node

Is it possible to change the compatibility matrix or is the the bug going to be fixed? So people won’t run into the same issues while using older driver versions.

Details:

Timer list being filtered by “isRunning”:

github.com

mongodb-js/mongodb-core/blob/master/lib/topologies/replset.js#L523


      
            return;
          }
          
          
if (self.state === DESTROYED || self.state === UNREFERENCED) {
            intervalId.stop();
            return;
          }
          
          
// Filter out all called intervaliIds
          self.intervalIds = self.intervalIds.filter(function(intervalId) {
            return intervalId.isRunning();
          });
          
          
// Initial sweep
          if (_process === Timeout) {
            if (
              self.state === CONNECTING &&
              ((self.s.replicaSetState.hasSecondary() &&
                self.s.options.secondaryOnlyConnectionAllowed) ||
                self.s.replicaSetState.hasPrimary())
            ) {

“isRunning” implementation using “_called”:

github.com

mongodb-js/mongodb-core/blob/master/lib/topologies/shared.js#L275


      
              return this;
            };
          
          
  this.stop = function() {
              clearTimeout(timer);
              timer = false;
              return this;
            };
          
          
  this.isRunning = function() {
              if (timer && timer._called) return false;
              return timer !== false;
            };
          }
          
          
function diff(previous, current) {
            // Difference document
            var diff = {
              servers: []
            };

Node.js pull request containing the removal of the “_called” variable (lib/timers.js line 282):

github.com/nodejs/node

timers: use only a single TimerWrap instance

nodejs:master ← apapirovski:patch-priority-queue

opened 05:09AM - 06 May 18 UTC

apapirovski

+469 -329

This PR moves almost the full entirety of timers into JS land and hangs them all… off one TimerWrap. This simplifies a lot of code related to timer refing/unrefing and significantly improves performance. ``` confidence improvement accuracy (*) (**) (***) timers/timers-breadth.js n=5000000 * 3.24 % ±2.84% ±3.78% ±4.92% timers/timers-cancel-pooled.js n=5000000 *** -16.38 % ±2.06% ±2.74% ±3.57% timers/timers-cancel-unpooled.js n=1000000 *** 47.81 % ±1.33% ±1.79% ±2.37% timers/timers-depth.js n=1000 -0.24 % ±0.89% ±1.19% ±1.56% timers/timers-insert-pooled.js n=5000000 *** 16.08 % ±7.09% ±9.49% ±12.47% timers/timers-insert-unpooled.js n=1000000 *** 162.84 % ±4.82% ±6.49% ±8.59% timers/timers-timeout-pooled.js n=10000000 *** 15.90 % ±1.59% ±2.11% ±2.76% timers/timers-timeout-unpooled.js n=1000000 *** 948.32 % ±21.44% ±28.89% ±38.36% ``` Fixes: https://github.com/nodejs/node/issues/16105  ##### Checklist - [x] `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes - [x] tests and/or benchmarks are included - [x] documentation is changed or added - [x] commit message follows [commit guidelines](https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines)

mbroadst · February 26, 2020, 4:30pm

Hi @Wilco_Waaijer!
Thanks for bringing this to our attention. I’ve actually already been working on a fix for this on NODE-2460, and hope to have a fix out today.

Regarding the compatibility matrix: we’re up to v3.5.4 of the driver now, so it’s unlikely that we will backport the fix three minor versions back. Generally we don’t backport changes within a major unless it is security related. In this case the recommendation is to upgrade to v3.5.x, where the fix will land shortly. I’ll get in touch with our docs team to correct the compatibility matrix.

Additionally, you can avoid this issue today by upgrading to v3.5.4 and using the “unified topology” by passing { useUnifiedTopology: true } to your MongoClient constructor, or useUnifiedTopology=true to your connection string.

Wilco_Waaijer · February 26, 2020, 7:08pm

Perfect. I was not aware that there was an issue for that already. We are upgrading and depending on the release of the fix look into the unified topology option.

It would indeed be nice if others are warned about the possible leak while running in Node.js v12 with the older versions. Is it okay if i comment on the issue with this thread or just the way to avoid the issue with the unified topology option? An extra way for people to find a solution if they’re searching for more info regarding the leak.

Thank you for taking the time to provide some more information!

mbroadst · February 26, 2020, 10:31pm

The issue has been fixed in this commit. Feel free to link to this page on the ticket.

We are looking for as much feedback as possible on the unified topology, since we’ll be completely removing the legacy topology types in the upcoming v4 major release. Please let us know if you decide to use it, and about your experience with it!