Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

connection closed on query but connect seems to work #985

Closed
DanielRuf opened this issue Jan 30, 2020 · 8 comments
Closed

connection closed on query but connect seems to work #985

DanielRuf opened this issue Jan 30, 2020 · 8 comments

Comments

@DanielRuf
Copy link

DanielRuf commented Jan 30, 2020

It seems the async / await version has some issue with the connect() method and running query() afterwards:

grafik

image

It is not clear why this happsn after some time when we start the application.

Expected behaviour:

query() should not fail as connect() was called before.

Actual behaviour:

query() fails due to closed connection to the SQl server.

Configuration:

// paste relevant config here

Software versions

  • NodeJS: 9.11.2
  • node-mssql: 6.0.1
  • SQL Server: ?
@DanielRuf
Copy link
Author

yes, the library does resolve even when not connected / closed

backendclient_1  | ConnectionPool {
backendclient_1  |   _events: {},
backendclient_1  |   _eventsCount: 0,
backendclient_1  |   _maxListeners: undefined,
backendclient_1  |   _connected: false,
backendclient_1  |   _connecting: true,
backendclient_1  |   _healthy: false,
backendclient_1  |   config: 
backendclient_1  |    { user: 'xxx',
backendclient_1  |      password: 'xxxx',
backendclient_1  |      server: 'xxxxxxx',
backendclient_1  |      database: 'xxx',
backendclient_1  |      options: { enableArithAbort: true },
backendclient_1  |      port: 1433,
backendclient_1  |      stream: false,
backendclient_1  |      parseJSON: false },
backendclient_1  |   close: [Function: bound globalClose] }

but no error, async / await successfully resolves this which is not correct imo

This is the result of db.connect() after the await.

@dhensby
Copy link
Collaborator

dhensby commented Jan 30, 2020

If you could actually provide the code and the errors and relevant configuration instead of poorly cropped screenshots, that would be infinitely more helpful in allowing me to assist you.

At the moment I have no idea what you are doing or why it's not working, though I can already see one fairly obvious problem...

@DanielRuf
Copy link
Author

though I can already see one fairly obvious problem...

Which would be? =)

We've found a workaround by retrying the request after the first connect - it seems the server takes some time to open the connection.

Perosnally I would have expected that connection.connected resolved always to true or throws an exception when it can not connect (hoist the connection error to reject), not resolve.

relevant configuration

It's in the first post, this is everything that we use (just the connection details).

The code snippet that already fails on our side:

class Database {
    async connect(){
        try {
            const connection = await db.connect({
                user: `${process.env.DB_USER}`,
                password: `${process.env.DB_PASS}`,
                server: `${process.env.DB_HOST}`,
                database: `${process.env.DB_DATABASE}`,
                options: {
                    enableArithAbort: true
                },
            });
            return connection;
        } catch (err) {
            throw new Error(error);
        }
    }

    // @route /login
    async login(username, password) {
        // @todo: implement
    }

    // @route /authenticated
    async authenticated(accessToken) {
        try {
            const connection = await this.connect();
            if (!connection.connected) {
                return { error: '-1' }
            }
            const result = await db.query...
...
ConnectionError: Connection is closed.
at Request._query (/srv/app/backendClient/node_modules/mssql/lib/base/request.js:447:37)
at Request._query (/srv/app/backendClient/node_modules/mssql/lib/base/request.js:346:11)
at shared.Promise (/srv/app/backendClient/node_modules/mssql/lib/base/request.js:413:12)
at new Promise (<anonymous>)
at Request.query (/srv/app/backendClient/node_modules/mssql/lib/base/request.js:412:12)
at Request._template (/srv/app/backendClient/node_modules/mssql/lib/base/request.js:76:26)
at Object.query (/srv/app/backendClient/node_modules/mssql/lib/global-connection.js:144:38)
at Database.authenticated (/srv/app/backendClient/js/database.js:34:42)

The query command fails after the connect because connection.connected resolved to false.

@dhensby
Copy link
Collaborator

dhensby commented Jan 30, 2020

The obvious mistake I can see is here:

        } catch (err) {
            throw new Error(error);
        }

error is not defined, err is.

As for your code:

  1. I don't know what db is in your code so I don't know how it should work
  2. I don't know how you are implementing this Database class you've authored
  3. It should not be possible for the connect() call to resolve if the connection is not ready unless you are calling the global connect function and you are calling it in parallel across many requests - then you will suffer from this problem: Connections all messed up? #961 (comment)

@DanielRuf
Copy link
Author

DanielRuf commented Jan 30, 2020

error is not defined, err is.

Thanks but this is not the problem =) It does not even throw there =)

db is mssql.

const db require('mssql')`

3\. It should not be possible for the connect() call to resolve if the connection is not ready _unless_ you are calling the global `connect` function and you are calling it in parallel across many requests - then you will suffer from this problem: [#961 (comment)](https://github.com/tediousjs/node-mssql/issues/961#issuecomment-570570890)

We use the async / await variant of mssql.connect().

So nothing parallel here.

@dhensby
Copy link
Collaborator

dhensby commented Jan 30, 2020

async / await still allows parallel calls...

You need to change your code to keep track of the fact the database is in a connecting state:

class Database {

  constructor() {
    this.connecting = false;
    this.connected = false;
    this.connection = Promise.reject('Call connect() first');
  }

  async connect() {
    if (!this.connected) {
      if (!this.connecting) {
        this.connecting = true;
        this.connection = db.connect({
          user: `${process.env.DB_USER}`,
          password: `${process.env.DB_PASS}`,
          server: `${process.env.DB_HOST}`,
          database: `${process.env.DB_DATABASE}`,
          options: {
            enableArithAbort: true
           },
        }).then(connection => {
          this.connected = true;
          this.connecting = false;
          return connection;
        });
      }
    }
    return this.connection;
  }

  async query(qry) {
    await this.connect();
    const result = await db.query(qry);
    return result;
  }

}

This enforces the tl;dr of the comment I linked to (tl;dr only call sql.connect() once in your application.)

@dhensby dhensby closed this as completed Jan 30, 2020
@DanielRuf
Copy link
Author

DanielRuf commented Jan 30, 2020

Many thanks, this does indeed work. =)

But retrying after this seems to work.

async / await still allows parallel calls...

I would expect that it waits until it is connected and using async / await in all places should wait until a method resolves or throws. I think this can be improved in this library (at least I would expect that the promise / then based version can be simply used with the async / await version) =)

Or this is not clearly documented.

@dhensby
Copy link
Collaborator

dhensby commented Jan 30, 2020

I'm not sure that async/await has anything to do with it - that doesn't make calls to promises blocking. If you're making repeat calls to mssql.connect() you're going to have a bad time. It is something that could be improved, but it's nothing to do with async / await, it's to do with the fact that subsequent calls to mssql.connect() will instantly resolve even if the original connection has not been established.

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

No branches or pull requests

2 participants