Node-mssql: Can not catch Error in Stream Mode

[Rafael-Dabrowski]

When trying to make a query with a problem in the query eg. wrong columnname while in Stream mode event.js throws an error which can not be catched. Subscription to the "error" event will be processed but the underlying error will still be thrown. The error can not be catched in the query callback nor promise and leads to App Crash.

Expected behaviour:

Error should only be thrown through error Eventhandler or Promise/Callback.

Actual behaviour:

Error is thrown in error Event and within events.js:

events.js:174
      throw er; // Unhandled 'error' event
      ^

RequestError: Incorrect syntax near the keyword 'Table'.

Configuration:

sql
  .connect(connection)
  .then(pool => {
    let request = pool.request();
    request.stream = true;
    let sql = "SELECT nonsense FROM Table";   
    let ws = fs.createWriteStream("output.txt");
    request.on("error", err => {
      console.log("Error in Stream", err.code);
    });
   request.pipe(through).pipe(ws);
   request.query(sql).catch(err => {
       console.log("error in Promise", err.code);
   });
});

Software versions

• NodeJS: 10.17.0
• node-mssql: 6.1.0
• SQL Server: 2017
• Driver: Tedious

[dhensby]

Does the error callback console.log("Error in Stream", err.code); get called?

If you've attached an error callback to the request, I wouldn't expect in error to be thrown.

Can you paste the full output when that example code is run, please?

[Rafael-Dabrowski]

Sure :)
Yes the Error in stream will be executed. The Error in Callback/Promise will not. I put a try/catch around the query and even the whole statement both will not be catched.

The Output is:

Error in Stream EREQUEST
events.js:174
      throw er; // Unhandled 'error' event
      ^

RequestError: Incorrect syntax near the keyword 'Table'.
    at handleError (C:\node\Demos\mssql\node_modules\mssql\lib\tedious\request.js:366:15)
    at Connection.emit (events.js:198:13)
    at Parser.tokenStreamParser.on.token (C:\node\Demos\mssql\node_modules\tedious\lib\connection.js:832:12)
    at Parser.emit (events.js:198:13)
    at Parser.parser.on.token (C:\node\Demos\mssql\node_modules\tedious\lib\token\token-stream-parser.js:37:14)
    at Parser.emit (events.js:198:13)
    at addChunk (C:\node\Demos\mssql\node_modules\readable-stream\lib\_stream_readable.js:298:12)
    at readableAddChunk (C:\node\Demos\mssql\node_modules\readable-stream\lib\_stream_readable.js:280:11)
    at Parser.Readable.push (C:\node\Demos\mssql\node_modules\readable-stream\lib\_stream_readable.js:241:10)
    at Parser.Transform.push (C:\node\Demos\mssql\node_modules\readable-stream\lib\_stream_transform.js:139:32)
Emitted 'error' event at:
    at Request.emit (events.js:203:15)
    at handleError (C:\node\Demos\mssql\node_modules\mssql\lib\tedious\request.js:369:16)
    at Connection.emit (events.js:198:13)
    [... lines matching original stack trace ...]
    at Parser.Readable.push (C:\node\Demos\mssql\node_modules\readable-stream\lib\_stream_readable.js:241:10)

[smferguson]

fyi, seeing this exact scenario as well

[dhensby]

@Rafael-Dabrowski in your code to reproduce you have not provided the definition of through, so your script does not run.

[dhensby]

OK - I've replicated the issue and there doesn't appear to be any problem in the library that I can see.

Even looking at your output it's clear the error event on the request stream is being emitted (your output starts with Error in Stream EREQUEST showing your error listener is catching the error).

The error that then comes through is being emitted by the writestream you are piping to, of which you have not added an error listener and therefore the error event is not caught. If you're going to pipe to another stream you're going to have to add the correct listeners to it just like you would for any other stream.

[smferguson]

If you're going to pipe to another stream you're going to have to add the correct listeners to it just like you would for any other stream.

that was my issue. thanks!

[Rafael-Dabrowski]

Oh wow, ok that i did not know. So i would also need to have an error listener on the piped streams aswell, i thought it was enough to have the error listener on the stream that throws/emits it. As errors will not be passed down the pipe.
Thank you.

[dhensby]

https://github.com/tediousjs/node-mssql/blob/v6.2.0/lib/base/request.js#L374-L383 - this is the current pipe behaviour. I don't know if this is "standard" or "normal" behaviour for piping streams but it's how it is implemented (not my code).

The stream implementation is non-standard and you can't assume it works as a native node Stream - there is an RFC for this #776

[dhensby]

According to this article https://medium.com/developers-arena/streams-piping-and-their-error-handling-in-nodejs-c3fd818530b6 the streams should not forward their errors up the pipe chain - so that's bad implementation on our side.

BUT you absolutely should be adding error listeners to all your streams regardless of whether we are forwarding errors. If your write stream errors for some reason, that needs to be handled and at the moment your implementation is not doing that.

So, whilst our implementation of pipe is bad/wrong, developers should still be handling an error even on their piped streams otherwise you will get the above behaviour when the write stream fails.

If the developer implements their piped stream correctly then our bad implementation will actually have very few repercussions so I don't see this as an important change to be making in the v6 line (but I'll look at fixing it for the 7.x line)