adonisjs / lucid

AdonisJS SQL ORM. Supports PostgreSQL, MySQL, MSSQL, Redshift, SQLite and many more
https://lucid.adonisjs.com/
MIT License
1.03k stars 189 forks source link

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

Closed jsilva74 closed 6 months ago

jsilva74 commented 1 year ago

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, }, },

`

pokedpeter commented 11 months ago

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

jsilva74 commented 10 months ago

always 30 seconds.

enixsoft commented 9 months ago

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 commented 9 months ago

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: https://github.com/adonisjs/lucid/blob/7e011509f656a683eaa2af8b7aedc14992edf2be/adonis-typings/database.ts#L508

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 commented 9 months ago

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

thetutlage commented 6 months ago

Closing, since fixed in https://github.com/adonisjs/lucid/issues/930.