Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cloudflare Client.end() hangs if socket did not successfully connect #3152

Open
rtbenfield opened this issue Feb 22, 2024 · 0 comments
Open

Comments

@rtbenfield
Copy link

There is a particular scenario where calling Client.end will result in a hanging promise when using Cloudflare's socket implementation and the socket does not successfully connect. I've found that this can be reproduced consistently by specifying an incorrect port in the connection string. This only occurs when using the Cloudflare runtime and CloudflareSocket.

// use a connection string with an unreachable port here
const client = new Client({ connectionString });
try {
  await client.connect();
  await client.query(`SELECT 1`);
} finally {
  // ⬇️ hangs here
  await client.end();
}

I believe I've found the root causes to it. The Socket.closed promise on the Cloudflare socket as used here does not resolve if the connection never successfully opened to begin with. Connection expects the event here to bubble out the end event to Client here and resolve the promise or invoke the callback.

Additionally, the stream.end callback is also not invoked in Connection.end. That's from the CloudflareSocket.write function expecting the callback to always be in the third argument here, where Connection.end passes it as the second argument here.

I made the following naive patches that resolve the issue in my case, though I'm not sure it's the best resolution. I'm expecting that successfully connected sockets would emit the close event twice with this patch.

diff --git a/node_modules/pg-cloudflare/dist/index.js b/node_modules/pg-cloudflare/dist/index.js
index e2db57e..f94b723 100644
--- a/node_modules/pg-cloudflare/dist/index.js
+++ b/node_modules/pg-cloudflare/dist/index.js
@@ -89,7 +89,8 @@ export class CloudflareSocket extends EventEmitter {
     end(data = Buffer.alloc(0), encoding = 'utf8', callback = () => { }) {
         log('ending CF socket');
         this.write(data, encoding, (err) => {
-            this._cfSocket.close();
+            // the closed Promise does not resolve if the connection was never open to begin with
+            this._cfSocket.close().then(() => this.emit('close'));
             if (callback)
                 callback(err);
         });
diff --git a/node_modules/pg/lib/connection.js b/node_modules/pg/lib/connection.js
index af4b8f1..af03e05 100644
--- a/node_modules/pg/lib/connection.js
+++ b/node_modules/pg/lib/connection.js
@@ -194,7 +194,7 @@ class Connection extends EventEmitter {
       this.stream.end()
       return
     }
-    return this.stream.write(endBuffer, () => {
+    return this.stream.write(endBuffer, undefined, () => {
       this.stream.end()
     })
   }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant