Node-mssql: failed query() is never settled

[MichaelKorn]

I have observed several times in my program that promises of queries that probably had to fail were never resolved.

Expected behaviour:

If a query() fails the Promises should be settled (be rejected).
In the case of a "Violation of PRIMARY KEY constraint" the query should fail.

Actual behaviour:

1. OK: If the query is fine (ID is not ins use), then the Promise is settled (fulfilled).
2. OK: If the query contains a syntax error, then the Promise is settled (rejected).
3. If the query is run a second time (the ID is already in use; Violation of PRIMARY KEY constraint), then the Promise is never settled (pending forever).

Configuration:

Minimal example:

const sql = require("mssql/msnodesqlv8");

database = 'myTestDB';
// Create test table with:
// CREATE TABLE [dbo].[TestTable](
//   [ID] [int] NOT NULL,
//   PRIMARY KEY (ID))

result = 1;

sql.connect({
  database: database,
  server: 'localhost',
  driver: 'msnodesqlv8',
  options: {
    trustedConnection: true
  }
}).then(async pool => {
  console.log('Connected');

  result = pool.query('INSERT INTO [TestTable] ([ID]) VALUES (1)');
  await result;
  console.log('Query resolved');

  return sql.close();
}).then(_ => console.log('fulfilled:', result), _ => console.log('rejected:', result));

Software versions

• NodeJS: v12.19.0
• node-mssql: 6.2.1
• msnodesqlv8: 2.0.5
• SQL Server: Microsoft SQL Server 2017 (RTM-GDR) (KB4505224) - 14.0.2027.2 (X64)

[dhensby]

There isn't officially support for msnodesqlv8 v2 in the v6.x branch of this project. Can you confirm if this behaviour also happens when using v1 of msnodesqlv8?

[MichaelKorn]

I can reproduce the issue with msnodesqlv8 1.1.8, too.

[Greensahil]

This error should be given high priority. Like @MichaelKorn said if the query fails because of Violation of PRIMARY KEY constraint or other reasons like if the input size is larger than field size it just waits and creates a lock on the table. I then have to deal with query timeout because the locks. Please put this on a high priority

[dhensby]

Hi @Greensahil, I'm happy for you to take ownership of the bug and provide a patch if it's a high impact issue for you.

Otherwise it will be resolved as and when someone has the time or willingness as this is an unfunded library maintained by myself (who is busy with a full time job and a personal life).

If the organisation you work for uses this free library perhaps they can contribute in kind to fix this problem?

[willmorgan]

If it's high priority, it _is_ easy to avoid multiple inserts on primary keys with some lightweight application logic.

[dhensby]

I can't replicated this problem.

Test script:

mssql.connect(sqlConfig).then((pool) => {
  console.log('connected')
  return pool.request().query('INSERT INTO [debug] ([ID]) VALUES (1)')
}).catch((err) => {
  console.error(err)
}).then(() => {
  console.log('resolved')
}).then(() => mssql.close())

or async for those of you that like it that way:

(async () => {
  const pool = await mssql.connect(sqlConfig)
  console.log('connected')
  try {
    await pool.request().query('INSERT INTO [debug] ([ID]) VALUES (1)')
  } catch (err) {
    console.error(err)
  }
  console.log('resolved')
  await pool.close()
})()

---

edit: just for clarity, output is:

{ RequestError: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot insert explicit value for identity column in table 'debug' when IDENTITY_INSERT is set to OFF.
    at handleError (.\lib\msnodesqlv8\request.js:266:21)
    at StreamEvents.emit (events.js:182:13)
    at errors.forEach.err (.\node_modules\msnodesqlv8\lib\reader.js:37:20)
    at Array.forEach (<anonymous>)
    at routeStatementError (.\node_modules\msnodesqlv8\lib\reader.js:30:14)
    at invokeObject.end (.\node_modules\msnodesqlv8\lib\reader.js:299:13)
    at freeStatement (.\node_modules\msnodesqlv8\lib\driver.js:175:13)
    at cppDriver.freeStatement (.\node_modules\msnodesqlv8\lib\driver.js:155:13)
  code: 'EREQUEST',
  originalError:
   { Error: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot insert explicit value for identity column in table 'debug' when IDENTITY_INSERT is set to OFF. sqlstate: '23000', code: 544 },
  name: 'RequestError',
  number: 544,
  state: '23000' }
resolved

[dhensby]

@MichaelKorn I tested your script too and it outputs as expected (obviously no "Query resolved" output nor does the pool get closed because the await result; line throws).

output:

rejected: Promise {
  <rejected> { RequestError: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot insert explicit value for identity column in table 'debug' when IDENTITY_INSERT is set to OFF.
      at handleError (.\lib\msnodesqlv8\request.js:266:21)
      at StreamEvents.emit (events.js:182:13)
      at errors.forEach.err (.\node_modules\msnodesqlv8\lib\reader.js:37:20)
      at Array.forEach (<anonymous>)
      at routeStatementError (.\node_modules\msnodesqlv8\lib\reader.js:30:14)
      at invokeObject.end (.\node_modules\msnodesqlv8\lib\reader.js:299:13)
      at freeStatement (.\node_modules\msnodesqlv8\lib\driver.js:175:13)
      at cppDriver.freeStatement (.\node_modules\msnodesqlv8\lib\driver.js:155:13)
    code: 'EREQUEST',
    originalError:
     { Error: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot insert explicit value for identity column in table 'debug' when IDENTITY_INSERT is set to OFF. sqlstate: '23000', code: 544 },
    name: 'RequestError',
    number: 544,
    state: '23000' } }

[dhensby]

Right, that "minimal example" doesn't actually trigger the reported error. Using this test script appears to result in the reported behaviour:

(async () => {
  const pool = await mssql.connect(sqlConfig)
  console.log('connected')
  const transaction = pool.transaction()
  try {
    const id = 1
    await transaction.begin()
    await transaction.request().query('SET IDENTITY_INSERT [debug] ON')
    await transaction.request().query(`INSERT INTO [debug] ([ID]) VALUES ('${id}')`)
    await transaction.request().query(`INSERT INTO [debug] ([ID]) VALUES ('${id}')`)
    await transaction.commit()
  } catch (err) {
    console.error(err)
    await transaction.rollback()
  }
  console.log('resolved')
  await pool.close()
})()

---

🤦‍♂️ - sigh, yes it does - just my debug table had the identity property turned on - the minimal test table doesn't.

[dhensby]

From my investigation this appears to be a bug (or at least unexpected behaviour) in the msnodesqlv8 driver.

When the driver emits the error event it has a flag "moreErrors" to indicate that there is more than one error being thrown by the driver. When this happens we don't resolve/reject the Promise until all the errors have come through, however in this instance we are seeing one error come through to the listener and then we see that an info event is emitted saying "the statement has been terminated" and there are no more error events emitted leaving the query hanging.

@timelorduk - any thoughts on what's going on here - I'm testing against v1.1.8.

Potential fix is to specifically handle that termination message and run the error handler when that happens

[TimelordUK]

Thanks for the information I will take a look. Unfortunately we won't support version 1 of library as we now run on Linux with version 2 however sounds like it's occurring on both. I'll keep you posted.