fix(parser): handle exceptions within handlePacket

[avallete]

Hey there !

This is a proposal, in order to address #2653 it introduce another error type in addition to DatabaseError named ParserError, in charge of bubbling up any unexpected error that might occurs not on the Database itself, but between the database and the javascript parsing code (such as string too long for javascript to handle).

This is not the perfect solution since the parser will still throw an error. The benefit of this is that the pg package will no longer throw uncatchable exceptions when faced with such cases.
Instead the error will bubble up into the same .on('error') channel and can be caught by the caller program.

One of the parsing error (string too long) can be reproduced with this database:

CREATE TABLE public.large_data(data text);
INSERT INTO public.large_data(data) VALUES (repeat('A', 750 * 1024 * 1024)); -- create a string larger than what JS can handle
-- within pg:  client.query(`SELECT * FROM public.large_data`)

[brianc]

thanks for this PR! is it possilbe to write a test for this?

[avallete]

@brianc I did my best but had to forge a specific invalid packet to force the parse error (since we can't reproduce the "max array error" in JS really).

I would like to make a more e2e test (ensuring within pg that the pooler/client now emit an error rather than an uncaught exception) but I can't find a way to do this without bumping the pg-protocol dependency. Do you have recommendation for how to proceed in that case ? I'm thinking I could do a commit by binding the "pg-protocol" lib to the local one so you can checkout and ensure it run. Keep the test skipped for now, deploy the pg-protocol next version and unskip the test.

Edit:

I want ahead and created the test in this commit: 65e7882

Can be tested locally by checking out on the branch, rebuilding pg-protocol, re-instaling deps for pg and running the tests.

Commented on the next commit, can be enabled once the new release of pg-protocol land.

[charmander]

I think this try is too broad, and that every individual message handler should be able to handle arbitrary data gracefully, with try only being introduced at the most granular level possible when arbitrary exceptions can occur (usually because of user-provided functions).

It also erases important information about the original error since it only preserves its string representation instead of the complete object, but that’s more easily solved.

[avallete]

Thanks for the comment!

Yeah, I totally agree with [this comment](#2653 (comment))

“uncatchable errors are always no bueno”

sums it up well.

So to me, it makes sense to have a catch-all at the top level, just in case something slips through in a handler. That way, worst case, it's still catchable and doesn't crash the app. As any exception raised here will result in uncatchable error within pg side otherwise.

Then, we can still work toward handling better each possible exception within each of the packet handler. But this seems like a related but different concern that can be handled on it's own.

About this part:

Also, https://github.com/brianc/node-postgres/issues/2653’s case can be a query error, same as if a type parser failed. It doesn’t need to be treated like a protocol error.

Not totally sure I follow — by "query error", do you mean something like a SQL error (e.g. missing table)? If so, that should already be returned as a DatabaseError, no? This won't throw an exception but return the error to the caller. But I guess you’re referring to something else, maybe you can clarify?

[charmander]

A generic protocol error is fatal for the connection, since it’s in an unknown state after the failure. A row field that can’t be converted into a JavaScript string because it’s too big doesn’t have to be this type of error. The ideal fix to #2653 would handle this condition by exposing it as a query error instead of a protocol error.

[charmander]

Also, #2653’s case can be a query error, same as if a type parser failed. It doesn’t need to be treated like a protocol error.