bug: Throws uncatchable exceptions
Closed this issue · 8 comments
If you pass headers that will later on result in misconfiguration errors when you try to query:
try {
trinoClient = lento({
hostname: undefined,
// this misconfiguration will later on cause an "uncatchable" error
headers: {
'X-Trino-User': undefined,
},
protocol: 'https:',
port: 443,
catalog: 'production',
schema: 'my_schema',
});
} catch (error) {
// this is never hit
console.error('Trino connection error', error);
return;
}Then later on when you query it throws an uncaught exception, even if the user tries to catch it in all possible places:
let result: any;
try {
// this "triggers" the uncatchable error due to the previous misconfiguration
result = (await trinoClient.query('select 1')) as any;
} catch (error) {
// this is also never hit
logger.error('Query failed', { query, error });
return
}This is because internally you perform the connection itself in a microtask without catching your own errors. You should catch your errors, and reject the promise that is actually returned to your user.
Note - you do properly reject the promise you return if the query fails, the bug is only if the connection itself fails, but your logic to establish the connection is "lazy" and won't run until you try to send a query.
Process exits 1 with:
TypeError: Invalid value "undefined" for header "x-trino-user"
at ClientRequest.setHeader (node:_http_outgoing:579:3)
at new ClientRequest (node:_http_client:256:14)
at Object.request (node:https:353:10)
at exports.AsyncResource.runInAsyncScope (node:async_hooks:201:9)
at Client._makeRequest (/Users/jribakoff/my-project/node_modules/lento/lib/client.js:135:60)
at Client.request (/Users/jribakoff/my-project/node_modules/lento/lib/client.js:103:10)
at QueryStream._takeoff (/Users/jribakoff/my-project/node_modules/lento/lib/query-stream.js:125:18)
at QueryStream._taxi (/Users/jribakoff/my-project/node_modules/lento/lib/query-stream.js:118:12)
at QueryStream._read (/Users/jribakoff/my-project/node_modules/lento/lib/query-stream.js:109:12)
at QueryStream.Readable.read (/Users/jribakoff/my-project/node_modules/lento/node_modules/readable-stream/lib/_stream_readable.js:456:10)
at resume_ (/Users/jribakoff/my-project/node_modules/lento/node_modules/readable-stream/lib/_stream_readable.js:913:12)
at processTicksAndRejections (node:internal/process/task_queues:83:21)
For example if you're doing this in an http (Express) route in a Kubernetes pod, instead of being able to return an http status code the whole pod crashes, and the client receives a "network error" (hopefully that pod wasn't serving any other important requests and you have retries setup properly and redundant pods!). It is also not great, because the user will initially think they are the ones doing something wrong but in fact the bug is in Lento itself.
I could argue that it's a bug in the application, for providing an invalid header. So letting it crash has my preference. But I'm an old dinosaur that looks at Promises with a mixed sense of curiosity and dread, and I should not be making the choice for you.
So, a PR is welcome!
It's understood the caller is passing invalid options. That doesn't mean the library doesn't have a bug. This isn't a mutually exclusive thing?
It's still a crash in the library code that crashes the whole process, instead of allowing the caller to handle the error. It doesn't matter if it's using callbacks or promises, the convention is normally to throw an error that can be caught (or with async code, either reject the promise or call the err callback), not crash the entire server. The library is scheduling a timeout that crashes the whole process, this is undesirable. A server can handle more than one request, just because one request fails does not mean the whole server should crash.
FYI, it also crashes on this error:
error: PrestoError: CATALOG_NOT_FOUND: line 15:10: Catalog 'foo' does not exist
at /app/.yarn/cache/lento-npm-2.4.0-cde0f3e4f8-5cf6f0371c.zip/node_modules/lento/lib/client.js:216:27
at Gunzip.<anonymous> (/app/.yarn/cache/simple-concat-npm-1.0.1-48df70de29-4d211042cc.zip/node_modules/simple-concat/index.js:8:13)
at Object.onceWrapper (node:events:645:28)
at Gunzip.emit (node:events:526:28)
at endReadableNT (node:internal/streams/readable:1345:12)
at processTicksAndRejections (node:internal/process/task_queues:83:21)
Note you don't crash the whole process for other forms of errors, like an invalid SQL statement. If you think this is "correct" why not always crash the process on any error then?
I should not be making the choice for you.
So, a PR is welcome!
Did you read this part of my comment? It's not necessary for us to agree on what is the right way to handle errors. Send a pull request and I'll gladly merge it.
😕 Shouldn't we be aligned on what the desired behavior is before we go changing any code?
Thanks for closing the issue without comment, I'll go ahead and keep my fix for myself.
I closed it with a fix. Released in 2.4.2.
Ah sorry, seeing that now. Thanks! Will take a look!
Confirming this fixed the issue, thanks again and sorry for the passive aggressive comment. I just 1) didn't want to try to modify your code if we weren't aligned and 2) incorrectly assumed you were passively aggressively closing the issue