Skip to content
This repository was archived by the owner on Feb 4, 2022. It is now read-only.

Improve handling for autoReconnect: false with timeouts #215

Merged
merged 3 commits into from
Oct 2, 2017
Merged

Improve handling for autoReconnect: false with timeouts #215

merged 3 commits into from
Oct 2, 2017

Conversation

vkarpov15
Copy link
Member

@adriaanmeuris and @bendytree 's repro for Automattic/mongoose#4513 helped uncover a few couple nasty edge cases with how mongodb-core handles autoReconnect: false.

First up, with the server topology, the below script will hang forever, because the count() call will wait forever for a reconnect.

const mongodb = require('mongodb');

run().catch(error => console.error('Uncaught error', error.stack));

async function run() {
  const db = await mongodb.MongoClient.connect('mongodb://localhost:27017/test', {
    socketTimeoutMS: 500,
    poolSize: 1,
    autoReconnect: false
  });

  db.on('timeout', function() {
    console.log('Got timeout');
  });

  db.on('disconnected', function() {
    console.log('Disconnected');
  });

  await db.dropDatabase();

  await db.collection('Test').insertOne({});

  try {
    await db.collection('Test').find({ $where: 'sleep(2000) || true' }).toArray();
  } catch(error) {
    console.log('Got error', error);
  }

  try {
    console.log('Before');
    await db.collection('Test').count();
    console.log('After', docs);
  } catch(error) {
    console.log('Got 2nd error', error);
  }

  console.log('done');
}

There's a similar issue with cursor, it'll just wait forever for a reconnect that the driver will never attempt.

const mongodb = require('mongodb');

run().catch(error => console.error('Uncaught error', error.stack));

async function run() {
  const db = await mongodb.MongoClient.connect('mongodb://localhost:27017/test', {
    socketTimeoutMS: 500,
    poolSize: 1,
    autoReconnect: false
  });

  db.on('timeout', function() {
    console.log('Got timeout');
  });

  db.on('disconnected', function() {
    console.log('Disconnected');
  });

  await db.dropDatabase();

  await db.collection('Test').insertOne({});

  try {
    await db.collection('Test').find({ $where: 'sleep(2000) || true' }).toArray();
  } catch(error) {
    console.log('Got error', error);
  }

  try {
    console.log('Before');
    await db.collection('Test').find().toArray();
    console.log('After', docs);
  } catch(error) {
    console.log('Got 2nd error', error);
  }

  console.log('done');
}

@vkarpov15
Copy link
Member Author

@mbroadst any thoughts on this?

@mbroadst
Copy link
Member

@vkarpov15 generally LGTM, my only hesitation is that there are no accompanying tests. Have you tried reproducing this with the mock server at all? It should be relatively easy to exercise this code path that way.

@vkarpov15
Copy link
Member Author

I haven't worked with the mongodb-core tests in a while so I was hesitant to dig into that. I'll have some time to circle back and look later this week.

@mbroadst
Copy link
Member

@vkarpov15 the testing story on the 3.0.0 branches is much improved, perhaps we could compromise on merging into both branches but only tests there 😄 Either way, I promise I'll take a look at this as I have time after 3.6 features are complete if you don't find the time.

@vkarpov15
Copy link
Member Author

@mbroadst got your tests ☝️ very tricky to get right but finally got it working, please review 👍

@mbroadst
Copy link
Member

mbroadst commented Oct 1, 2017

@vkarpov15 rad! Thanks for going through the trouble, I really appreciate that.

@vkarpov15
Copy link
Member Author

Cool. When can we get this merged so I can close out Automattic/mongoose#4513?

@mbroadst mbroadst merged commit e6ad582 into mongodb-js:2.0 Oct 2, 2017
@vkarpov15 vkarpov15 deleted the fix-autoreconn branch October 2, 2017 16:33
@vkarpov15
Copy link
Member Author

Any ETA on release? Don't mean to nag, just eager to close out the issue. Also, re: discussion a few weeks back, here's a writeup on some learnings about connection events in the node driver

@mbroadst
Copy link
Member

mbroadst commented Oct 3, 2017

@vkarpov15 by end of week. I want to make sure to back out anything from #220 before the release, it seems that a memory leak was introduced there for sure. Thanks for that blog post, I'll make sure to read through it!

@mbroadst
Copy link
Member

@vkarpov15 v2.1.16 and v2.2.32 are released 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants