Connection do not close after call sequelize.close()

[mkovel]

Hi I faced with issue of closing connecting to db after calling of sequelize.close().

You can find below example of source code with reproducing this issue.
I also prepared pull request #8 that fix this issue, please review it.

const Sequelize = require('sequelize');

process.on('exit', () => {
  console.log('process.on exit');
});

const sequelize = new Sequelize('mysql://user:pass@localhost:3306/dbname', {
  dialect: 'mysql',
});

async function cleanDB() {
  await sequelize.authenticate();
  console.log('DB connection successfully established');
  return sequelize.transaction(async t => {
    const options = {
      transaction: t,
    };
    await sequelize.query('SELECT 1+1', options);
    console.log('Query was processed successfully');
  });
}

cleanDB()
  .then(() => {
    console.log('cleanDB finish successfully ');
  })
  .catch(err => {
    console.log('cleanDB throw error => ', err);
  })
  .finally(() => {
    sequelize
      .close()
      .then(() => {
        console.log('Connection has been close successfully.');
      })
      .catch(err => {
        console.error('Unable to close connect to the database:', err);
      });
  });

[sushantdhiman]

sequelize.close() returns a promise. Try adding return to finally

[mkovel]

Do you mean like this ?

....
cleanDB()
  .then(() => {
    console.log('cleanDB finish successfully ');
  })
  .catch(err => {
    console.log('cleanDB throw error => ', err);
  })
  .finally(() => {
     return sequelize .close();
  });

It did not help.
Because issue in this part of source code in /lib/Pool.js destroy() method.

....
  destroy(resource) {
    const available = this._availableObjects.length;
    const using = this._inUseObjects.length;

    this._availableObjects = this._availableObjects.filter(
      object => object.resource !== resource
    );
    this._inUseObjects = this._inUseObjects.filter(
      object => object !== resource
    );

    // resource was not removed, then no need to decrement _count
    if (
      available === this._availableObjects.length &&
      using === this._inUseObjects.length
    ) {
      this._ensureMinimum();
      return;
    }

    this._count -= 1;
    if (this._count < 0) this._count = 0;

    this._factory.destroy(resource);
    this._ensureMinimum();
  }

When Pool.destroy() is calling from Pool.destroyAllNow() this._factory.destroy(resource); will never be executed and connection resource will not be destroyed.

Please take a look on my variant of sultion this issue in this PR

[herebebogans]

+1

We have the same issue with sequelize.close() no longer closing the db connection with sequelize 5.9.8 which included a major version bump in this library.
Same code works fine in sequelize 5.9.7.

[mkovel]

@herebebogans Yes, problems joined after sequelize 5.8.8. Because sequelize 5.9.8 have has updated dependency with sequelize-pool v 2.*

[sushantdhiman]

Can you guys submit a case for this library not sequelize. drain / destroyAllNow should be able to issue destroy at least once

[mkovel]

@sushantdhiman destroyAllNow () does not destroy resources, because at the beginning of the method a pool is copied, and then this_availableObjects = []; is cleared.

  destroyAllNow() {
    this._log("force destroying all objects", "info");

    const willDie = this._availableObjects;
    this._availableObjects = [];
    const todo = willDie.length;

    this._removeIdleScheduled = false;
    clearTimeout(this._removeIdleTimer);

  ...
  }

and inside of destroy() if construction will always invoked

  destroy(resource) {

    // resource was not removed, then no need to decrement _count
    if (
      available === this._availableObjects.length &&
      using === this._inUseObjects.length
    ) {
      this._ensureMinimum();
      return;
    }
  }

[mkovel]

@sushantdhiman please take a look at my PR i have fixed this issue already #7 Connection do not close after call sequelize.close()

[sushantdhiman]

Got it, Not too happy with force option. Will think about something else

[mkovel]

Do you worry about name of option or about solution in general ?

[sushantdhiman]

about solution in general

Yes, that is why I need some time to think :)

[olmaygti]

Hi @sushantdhiman, I agree that the force option is not a good idea, adding these kind of ad-hoc shortcuts to solve very specific problems can only make the codebase to become spaguetti code. (No offense @mkovel :D).

Please take a look at my PR, which tries to solve the problem in its origin by not changing the _availableObjects variable until those objects are already destroyed, letting the destroy method do what it has to do.

It also takes into account that methods such as _factory.destroy(), which is provided by an external party, might be (and will very likely be) asynchronous operations.

If you agree with the approach of this PR, I'd like to continue contributing to the codebase by making methods like _ensureMinimum() or _createResource() "asynchronous aware". We've just started using sequelize in my company and we need this to be bullet proof, so we'd be happy to contribute with anything that might be needed.

Thanks!.

[bagi091]

@olmaygti Your solution is not provide creating a new clients during the invoking the destroyAllNow.

[olmaygti]

Hi @bagi091, sorry but I don't understand your comment.

• If you rely on the pool to create the configured minimum number of instances when destroyAllNow() is called, this is still happening. The call to _ensureMinimum() still takes place and I haven't modified its implementation, so that part should work as it was before. If you have verified it's not please let me know.

• If your point is that this should not happen, I'm waiting for feedback in my PR to know whether if I should change that behavior or not. Will be happy to do it if requested.

[sushantdhiman]

This issue has been fixed by sequelize-pool@2.2.0 and sequelize@5.8.12. Let me know if this needs any improvements