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

In v5, Database timeout config not being respected with MS Sql Server #930

Closed
jsilva74 opened this issue Apr 17, 2023 · 6 comments
Closed

Comments

@jsilva74
Copy link

Hi.

I have the following database config which set up connection timeout what isn't respected.

Appreciate any help. Thanks.

Package version

5.8.6

Node.js and npm version

node v16.19.0
npm 8.19.13

Sample Code (to reproduce the issue)

`
mssql: {
client: 'mssql',
connection: {
user: Env.get('MSSQL_USER'),
port: Env.get('MSSQL_PORT'),
server: Env.get('MSSQL_SERVER'),
password: Env.get('MSSQL_PASSWORD', ''),
database: Env.get('MSSQL_DB_NAME'),
connectionTimeout: 120000,
requestTimeout: 120000,
},
pool: {
min: 2,
max: 20,
},
migrations: {
naturalSort: true,
},
},

`

@Julien-R44 Julien-R44 transferred this issue from adonisjs/core Apr 30, 2023
@pokedpeter
Copy link
Contributor

What timeout value are you getting? and is it consistent?

@jsilva74
Copy link
Author

always 30 seconds.

@enixsoft
Copy link

I can confirm this is still an issue with:
@adonisjs/lucid 18.4.0
@adonisjs/core 5.8.6

There is an workaround, you need to define MS SQL database connection like this:

    mssql: {
      client: 'mssql',
      connection: {
        server: Env.get('DB_SERVER'),
        port: Env.get('DB_PORT', 1433),
        user: Env.get('DB_USER'),
        password: Env.get('DB_PASSWORD'),
        database: Env.get('DB_NAME'),
        options: {
          // @ts-ignore eslint-disable-next-line
          requestTimeout: Env.get('DB_REQUEST_TIMEOUT'),
        },
        connectionTimeout: Env.get('DB_CONNECTION_TIMEOUT'),
        requestTimeout: Env.get('DB_REQUEST_TIMEOUT'),
      },
    },

Basically requestTimeout needs to be in options object despite TypeScript definitions suggesting that is not the right way. I am not sure why it does not work, code seems okay and connectionTimeout works correctly.
See here: https://github.com/knex/knex/blob/master/lib/dialects/mssql/index.js#L57

@pokedpeter
Copy link
Contributor

It looks like Knex, at some point, switched from using the node-mssql client to the tedious driver.
https://github.com/knex/knex/blob/bbbe4d4637b3838e4a297a457460cd2c76a700d5/UPGRADING.md?plain=1#L120

Adonis' config is still using the one straight from node-mssql:

requestTimeout?: number

So I reckon a fix would be to update the MssqlConnectionNode type in the above file with the connection config in tedious:
https://github.com/tediousjs/tedious/blob/4f3e2109128a5291675aba5f73a0b56aa8016d2f/src/connection.ts#L333

@enixsoft
Copy link

@pokedpeter I have already found and created a pull request for the most likely source of this issue: thetutlage/knex-dynamic-connection#24

@thetutlage
Copy link
Member

Closing, since fixed in #930.

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

4 participants