search

chilltemp / tedious-promises

14 stars 7 forks source link

tedious promises doesn't listen to error event and can cause node to crash #3

Open oudsteyn opened 8 years ago

oudsteyn commented 8 years ago

events.js:141 throw er; // Unhandled 'error' event ^ ConnectionError: Login failed for user 'PoweredApp'. at ConnectionError (/Users/c/BitBucket/powered-server/node_modules/tedious/lib/errors.js:21:12) at Parser. (/Users/c/BitBucket/powered-server/node_modules/tedious/lib/connection.js:256:66) at emitOne (events.js:77:13) at Parser.emit (events.js:169:7) at Parser. (/Users/c/BitBucket/powered-server/node_modules/tedious/lib/token/token-stream-parser.js:51:15) at emitOne (events.js:77:13) at Parser.emit (events.js:169:7) at readableAddChunk (/Users/c/BitBucket/powered-server/node_modules/readable-stream/lib/_stream_readable.js:198:18) at Parser.Readable.push (/Users/c/BitBucket/powered-server/node_modules/readable-stream/lib/_stream_readable.js:157:10) at Parser.Transform.push (/Users/c/BitBucket/powered-server/node_modules/readable-stream/lib/_stream_transform.js:123:32)

chilltemp commented 8 years ago

This is because tedious-connection-pool requires a subscription to the 'error' event. I don't think that tedious-promises should subscribe to this event, as it can't really do anything with it and it will already handle errors from acquire().

Although it's probably a good idea to subscribe to this event, I've submitted a pull request to relax the requirement. You can wait for the updated version or avoid the problem by subscribing to the event before passing the connection pool to tedious-promises.

ben-page commented 8 years ago

I'm the author of tedious-connection-pool.

The error event is not simply a duplicate of errors that are based to acquire(). The err parameter in acquire() receives only errors related to the call to acquire(). Such as when the acquire timeout value is exceeded.

The error event is raised when one of the pooled connections experiences a socket or protocol error, such as losing the connection to the server. This can be emitted at any time, like before you call acquire() (while the connection is waiting in the pool) or after you have acquired the connection (but before you've released it). Ultimately, the connection to the SQL Server can fail at anytime, even before the acquire Promise was created or after the Promise is resolved. Often, events don't map well to Promises. Events can fire multiple times and at any time. Promise can only be resolved once and can't change their value.

The error event simply doesn't map well to a Promise API. I love Promises, but they are not a solution to every problem. This challenge isn't unique to tedious-connection-pool. The build in HTTP server operates exactly the same way. If you promisified it, you would still need to handle the error event. Users of your library will still need to subscribe to the error event. Or they can let the UncaughtException event crash the application and use a process monitor to restart it.

chilltemp commented 8 years ago

Ok. Thanks for the info. I did not realize that aquire did not re-throw the original connection errors.

Also, I can see scenarios where I would want the errors as they happen, and scenerios where I wouldn't care unless/until it interfered with one of the commands I attempt to run.

My first thought is to provide and optional (not default) way to collate the errors and return them the next time a Connection or Request fails. But as you indicate, it would not be a perfect solution. I'll need to think on this some more before I decide if there is a reasonable and useful way to promiseify this.

ben-page commented 8 years ago

Conceptually, one could imagine catching the error and returning it the next time the user does something that uses the connection. But in practice, this is not a simple task.

Remember the error originates from the tedious connection and once that error is emitted, tedious expects you to stop using the connection. Continuing to use it in anyway will have unpredictable results. tedious-connection-pool couldn't really implement this type of solution. The entire tedious public interface (every method and every property) would need to be updated to rethrow the error or pass it to the callback. And then your users would have to wrap every synchronous interaction with a try/catch statement or you'd have to promisify all the synchronous stuff just to catch the errors.