neondatabase / serverless

Connect to Neon PostgreSQL from serverless/worker/edge functions
https://www.npmjs.com/package/@neondatabase/serverless
MIT License
321 stars 13 forks source link

Timeout instead of error if the user did not configure the database URL #30

Closed kelvich closed 1 year ago

kelvich commented 1 year ago

@skyzh's report:

The serverless driver will timeout instead of reporting any error if the user did not configure the database URL. It seems that the driver will try connecting to localhost:5432 if the database URL is not configured, and it will timeout. This looks like the default behavior for pg library but I don’t think it makes sense if people are using the serverless driver as they are always connecting to an external provider? I feel it would be better to give a warning / throw an error in the serverless driver in case that the database endpoint is not configured?

jawj commented 1 year ago

Ah, good point. We already console.warn on this for the WebSocket transport: https://github.com/neondatabase/serverless/blob/1477880489f520068fb5940b042dd273a2dfedf1/export/index.ts#L61

We don't currently do so for http fetch, but I'll add it in.

skyzh commented 1 year ago

Actually I think we should error instead of warn, because if it times out in Vercel Edge Functions, it does not show any log. All I can see is like EDGE_FUNCTION_INVOCATION_TIMEOUT.

image
jawj commented 1 year ago

I haven't deployed the change yet. Shall we wait to see if the warning shows up in the Vercel console once I have?

jawj commented 1 year ago

(There may some cases where it's useful to query localhost in development — I know people are doing that with the WebSocket transport, at least).

skyzh commented 1 year ago

Sure, I'll try it out once you release a new version on npm. (I've tried using "@neondatabase/serverless": "github:neondatabase/serverless" in my packages.json to directly install from git but it does not work for me, unluckily.)

jawj commented 1 year ago

Right — unfortunately the npm package is deployed only from a subfolder of the GitHub repo, so installing from "github:neondatabase/serverless" won't work. Arguably I should see about changing that one of these days.

Anyway, version 0.5.0 of the driver, just released, should warn if the host is localhost for http as well as WebSocket queries. It would be great if you could check if that's working.

skyzh commented 1 year ago

Yeah I now get the warning locally, but it seems that it still times out on Vercel so that users cannot find the warning in Vercel console. The only thing I saw in Vercel is like:

[GET] /api/reactions/get/?slug=test reason=EDGE_FUNCTION_INVOCATION_TIMEOUT, status=504, user_error=true

My edge function is like:

        const pool = new Pool({ connectionString: process.env.DATABASE_URL });
        const db = new Kysely<DB>({ dialect: new PostgresDialect({ pool }) });

        const query = /* do something */

        const reactions = await query.executeTakeFirst();
        ctx.waitUntil(pool.end());

I wonder if this piece of code will stuck somewhere (ctx.WaitUntil?) if connectionString is set to undefined due to env variable not available.

skyzh commented 1 year ago

i.e., probably on Vercel connecting to a localhost port will not end up with connect ECONNREFUSED ::1:443 but it will stuck there?

jawj commented 1 year ago

OK — I now realise this is a WebSocket issue, not an http issue.

So I had a bit more of a think about it, and as of 0.5.2 we now throw an error if we detect default connection parameters.

Anyone who actually wants the defaults can override this behaviour by explicitly setting a host or connectionString parameter, or the PGHOST environment variable.

Again, if you'd be willing to confirm whether this fixes the problem for you, that would be much appreciated. :)

skyzh commented 1 year ago

Yep, now it works perfectly for me, thanks a lot!