Connections all messed up?

[ghost]

The way connections are handled doesn't seem to be well defined. Examples seem to open connections and not close them. Copying example code to a simple js file will leave node hanging because the connection doesn't close.

Also a 'ConnectionPool' doesn't seem to work like expected either. If you create a ConnectionPool, the name seems to imply that there is a pool of connections that will allow you to execute multiple commands at once. In testing, this does not seem to be the case.

From what I can tell, there is only ever a single global connection to sql server. The error I get lends weight to that case, no matter how I am connecting or how many 'ConnectionPool's I use:

Global connection already exists. Call sql.close() first

Expected behaviour:

1. Documentation and samples need to be improved and to represent best practices for using the package. That includes closing the connection, and especially some kind of warning that there is only one global connection.

2. There should be multiple connections allowed. Having the library be async doesn't make a lot of sense if you have to write your own code to make sure you never attempt to run two queries or create two connections at the same time or you will get an error.

Actual behaviour:

Creating a new connection, connection pool, request, or anything seems to cause an error:

Global connection already exists. Call sql.close() first.

Configuration:

// paste relevant config here

Software versions

• NodeJS: 8.11.1
• node-mssql: 5.1.0
• SQL Server: 16

[willmorgan]

Thanks for raising this, Jason.

ConnectionPooling is widely misunderstood: https://en.wikipedia.org/wiki/Connection_pool

Multiple connections are allowed. You will need multiple connections open in the pool to run multiple requests against them.

You haven't provided any example usage in this ticket. Documentation updates are always welcome, but "all messed up" is not constructive so I will be closing this ticket.

[dhensby]

Hi @JasonGoemaatAtCorteva - I'm afraid it seems like you're using the library incorrectly. There are many connections allowed (many connection pools does not mean the same as many connections, but even that is fine).

As @willmorgan has said, we're happy to take in improved documentation pull requests, but the reality is you need to have a grasp of connection pooling before starting to say the docs are wrong (we can't explain every concept to the finest detail in our docs).

You absolutely can make multiple concurrent requests over many connections with this library but trying to do it with multiple pools is not the right approach.

If you upgrade to v6 you'll be able to call mssql.connect() repeatedly if you don't want to worry about pool management.

Examples seem to open connections and not close them. Copying example code to a simple js file will leave node hanging because the connection doesn't close.

the docs don't show the pool being closed because the pool should only be closed when the application is shutting down and our examples only show how to run one-off queries, if you close the pool after every query, you're losing all the advantages of pooling and adding a massive overhead to requests. There is no point creating examples that are fully self-sufficient when copied into a JS file, this is pointless as its not how the library is used, the docs are there to give a flavour of how to use the library, they are not examples of a standalone application.

Also a 'ConnectionPool' doesn't seem to work like expected either. If you create a ConnectionPool, the name seems to imply that there is a pool of connections that will allow you to execute multiple commands at once. In testing, this does not seem to be the case.

Incorrect, it absolutely is the case and works as expected, the following code will execute the 4 queries in parallel across 4 connections.

const pool = new ConnectionPool(config);

pool.connect().then(connectedPool => {
  return Promise.all([
    connectedPool.query('SELECT 1'),
    connectedPool.query('SELECT 1'),
    connectedPool.query('SELECT 1'),
    connectedPool.query('SELECT 1')
  ])
}).then(results => console.log).then(() => pool.close())

From what I can tell, there is only ever a single global connection to sql server.

not true, there is only ever one global connection pool at a time (accessed via mssql.connect()) which must be closed before trying to create a new global pool. in v5 you'll need to keep a reference to that pool and not call mssql.connect() again until it's been closed. Upgrade to v6 for slightly nicer behaviour.

There is a slight misnomer here as it's not a global connection, it's a global pool, but it's naming doesn't change what it is.

1. Documentation and samples need to be improved and to represent best practices for using the package. That includes closing the connection, and especially some kind of warning that there is only one global connection.

Yep, there could be some improvements over the docs for the global connection pool.

2. There should be multiple connections allowed. Having the library be async doesn't make a lot of sense if you have to write your own code to make sure you never attempt to run two queries or create two connections at the same time or you will get an error.

Multiple connections are allowed, nothing in the library stops multiple connections, multiple connections to the DB are created all the time during normal use of the library, see previous example.

Creating a new connection, connection pool, request, or anything seems to cause an error:

That's not correct, see previous example.

Lastly, please provide example code of what's "not working", how can we help if we don't know what you are doing? We just have to assume you're incorrect usage when someone reports that multiple connections are not allowed in a library literally designed to allow multiple connections to the DB through the pools. My assumption at the moment is you are repeatedly calling mssql.connect().

[dhensby]

Docs improvements merged

[ghost]

Thank you for the response. Sorry I didn't add specific code, but just using the samples in the docs didn't work. I threw together a simple test app... So say I have a node app that is a simple REST api making a single sql query for each call to the api. What would be the correct way to use the library?

Here is the sample code I used for testing with the first code sample in the docs:

const sql = require('mssql')
const config = {...};
const query = 'select top(1) * from MYTABLE';

const quickExample = async () => {
     try {
        // make sure that any items are correctly URL encoded in the connection string
        await sql.connect(config)
        const result = await sql.query(query);
        console.log('quickExample', result);
    } catch (err) {
        console.error('quickExample error:', err);
    }   
}

let func = quickExample;

promises = [1, 2, 3, 4, 5, 6].map(index => func(index).catch(err => console.log(`err with index ${index}!`, err)));

Promise.all(promises)
.then(results => {
    console.log("Done with all!");
})
.catch(err => {
    console.log('Errors with all!');
    console.log(err);
});

That fails for 5/6 of the function calls with ConnectionError: Connection is closed. and node never exits, I think because the connection is open.

Using the second code sample but changing it to use simple queries has the same result. Both queries return results for one of the runs, but 5/6 fail with the same error:

const asyncAwait = async () => {
    try {
        let pool = await sql.connect(config)
        let result1 = await pool.query(query);
        let result2 = await pool.query(query);
        console.dir('asyncAwait result1:', result1);
        console.dir('asyncAwait result2:', result2);
    } catch (err) {
        console.error('asyncAwait error:', err);
    }
}

let func = asyncAwait;

So from the first examples it seems that only one connection can be open and I should not close it, but have to manage keeping more than one query from executing at a time. Maybe call sql.connect() once? What if the network gets disconnected and the connection closes, how can that be handled?

Moving on to the callbacks sample, the same thing happens:

const callbacks = () => {
    return new Promise((resolve, reject) => {
        sql.connect(config, err => {
            if (err) {
                console.log('callbacks error:', err);
                reject(err);
            }

            
            new sql.Request().query('select 1 as number', (err, result) => {
                if (err) console.log('callbacks first query error:', err);
                console.dir('callbacks first query result:', result);
            })
        
            const request = new sql.Request()
            request.query(query, (err, result) => {
                if (err) console.log('callbacks second query error:', err);
                console.dir('callbacks second query result:', result);
            })
        });
        
        sql.on('error', err => {
            console.log('callbacks sql.on(error):', err);
        });
    });
};

let func = callbacks;

I see that I had installed the new version 6, when using 5.1 the results are the same but the error is different (Global connection already exists. Call sql.close() first),

Using the current version 6, I use what I think should work based on the first code sample under global connection pool and the same thing happens:

const globalConnectionPool = async () => {
    try {
        let pool = await sql.connect(config);
        let result = await pool.query(query);
        console.log('globalConnectionPool result:', result);
    } catch (err) {
        console.log('globalConnectionPool error:', err);
    }
};

let func = globalConnectionPool;

The docs state:

Using a single connection pool for your application/service is recommended.

So I tried that code:

const pool1 = new sql.ConnectionPool(config);
const pool1Connect = pool1.connect();
pool1.on('error', err => {
    console.log('pool1 error:', err);
});

const connectionPools = async () => {
    await pool1Connect;
    try {
        const request = pool1.request();
        const result = await request.query(query); // missing await in docs
        console.log('connectionPools result:', result);
    } catch (err) {
        console.log('connectionPools error:', err);
    }
};

let func = connectionPools;

That didn't work for me before either, maybe because of the missing await in the docs on request.query(). It seems to work now though. What exacly happens in a conenction pool? It's hard to understand why I have to await pool1Connect, shouldn't connections be happening automatically? Is there a certain number of connections that are opened at that time and that remain open? What happens if there is a network error and a connection is forcibly closed, is it re-opened automatically?

What I ended up doing is creating a new connection pool for each query, the way I was able to get it to work reliably... It has the added benefit for a console app especially that it exits when done because there isn't an open connection hanging on out there:

const newPool = async () => {
    const pool = new sql.ConnectionPool(config);
    const connect = await pool.connect();
    try {
        const request = await pool.request();
        const result = await request.query(query);
        console.log('newPool result:', result);
        pool.close();
        return result;
    } catch (err) {
        console.log('newPool error:', err);
        pool.close();
    }

};

let func = newPool;

[dhensby]

tl;dr only call sql.connect() once in your application.

In your examples where you are using the global connection pool (sql.connect(config)), you are making the same mistake repeatedly and you are not taking into account my example. You should only be calling sql.connect() once in your application's lifecycle. As shown previously:

const sql = require('mssql')

sql.connect().then(connectedPool => {
  return Promise.all([
    connectedPool.query('SELECT 1'),
    connectedPool.query('SELECT 1'),
    connectedPool.query('SELECT 1'),
    connectedPool.query('SELECT 1')
  ])
}).then(results => console.log).then(() => pool.close())

In your examples you are calling sql.connect() in parallel several times. When you call sql.connect() in parallel all but the first call will instantly resolve with the non-connected global pool (a bit of a quirk, which could be fixed but this is why you're getting 5/6 errors saying the connection is closed). I'm not sure how to make this any clearer stop calling connect repeatedly, call it once. Node never exits in your examples because you're never closing the connection pool so it keeps the node process running.

Again, you can execute queries in parallel and you don't need to do anything to ensure that your queries are only executed one at a time - not sure how to make that clearer. Again, the problem is you are repeatedly calling sql.connect().

What exacly happens in a conenction pool? It's hard to understand why I have to await pool1Connect, shouldn't connections be happening automatically? Is there a certain number of connections that are opened at that time and that remain open? What happens if there is a network error and a connection is forcibly closed, is it re-opened automatically?

It's too involved to tell you exactly what's happening in a connection pool - if you're interested, please read the code. As a light overview connection pools have a connect function that creates an initial connection to the DB to validate your connection config is correct and the SQL server is available. After this connections are created on demand within the bounds specified in the config (eg: min/max number of connections in the pool). When you run a query against a ConnectionPool it attempts to acquire a connection from the pool; Simple logic flow: if spare connection, use it, else if num connections < max allowed, create a connection, run query against connection, release connection back to pool.

Why do you have to await pool1Connect? So that you can be sure the pool has connected and so you don't get the error you're getting in your earlier examples (where the pool is closed because you haven't awaited the initial connection).

Is there a certain number of connections that are opened and remain open? Yes, this depends on your pool config (min/max values)

What if the connection errors? Then that connection is removed from the pool, but the query using that connection will also error. A new connection will be opened (when needed) if a new query is run against the pool as per above logic flow.

What I ended up doing is creating a new connection pool for each query, the way I was able to get it to work reliably... It has the added benefit for a console app especially that it exits when done because there isn't an open connection hanging on out there:

That's fine, it's very inefficient and is a very bad idea but it will "work". As explained before, if you want your node process to exit, then you have to close the global connection pool - of course in your final example it exits, because you're closing the pool(s), you need to do the same thing if you use the global connection pool as well.

I've investigated allowing repeat calls to connect() in parallel, but it opens all manner of edge cases that are hard to manage or that may be just as confusing as what we have at the moment, see: #941

[dhensby]

So say I have a node app that is a simple REST api making a single sql query for each call to the api. What would be the correct way to use the library?

Assuming you use expressjs - note this is rough, untested and just a gist of how to go about it:

const express = require('express')
const app = express()
const port = 3000
const sql = require('mssql')
let dbPool

app.get('/users', async (req, res) => {
  try {
    const results = await dbPool.query('SELECT * FROM users')
    res.json(results);
  } catch (err) {
    res.status(500).send('There was an error')
  }
})

sql.connect().then((pool) => {
  dbPool = pool
  app.listen(port, () => console.log(`Example app listening on port ${port}!`))
).catch(err => {
  console.error('failed to connect to DB')
  process.exit(1)
})

[vladakg85]

@dhensby In your last example why you didn't close connection/pool?

[dhensby]

why you didn't close connection/pool?

because you should only close the pool when the application quits / exits, not when it's finished dealing with a single request.

[cinnamonKale]

tl;dr only call sql.connect() once in your application.

Does this still apply, considering the docs say multiple sql.connect() are fine?

In my case, the server starts with the initial PoolConnection:

new sql.ConnectionPool(config).connect().then(pool => /** Start app */);

And routes in other files want to use that initial connection, or create a new PoolConnection if the database server dropped for some reason:

const routeHandler = () => { sql.connect(config).then(pool => /** Query */).catch(/** Error handler */); };

In this example, does the routeHandler connect use the existing pool? Or will calling sql.connect(config) actually create a new pool every time I need a connection?

If it's the latter, we would still need to manage a global.pool across the app, which would seem to contradict the docs.

Note, I'm only calling sql.close() once, at the end of the application. So each usage of the pool just runs sql.connect()

[dhensby]

Yes, there is now a feature that allows multiple calls to sql.connect() to be called. It is a bit naive in that if you pass different configs in subsequent calls to connect it will still return the original connection, but it's really intended to be a low effort/low feature helper for basic usage of the library. If you need to run connections to multiple databases / run multiple pools, then you should be managing this as part of your application state.

In this example, does the routeHandler connect use the existing pool? Or will calling sql.connect(config) actually create a new pool every time I need a connection?

It will not create a new pool every time any more (it never used to, I don't think, it would have thrown an error).