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

crash #45

Closed
hiddenhippo opened this issue Aug 27, 2011 · 7 comments
Closed

crash #45

hiddenhippo opened this issue Aug 27, 2011 · 7 comments

Comments

@hiddenhippo
Copy link

Really sorry if this isn't the place to post, but I've searched high and low for somewhere to see if someone can shed any light on the following issue.

I've compiled and built node.js version 0.4.11 under linux-mint Debian 201108 RC, and have used npm to install node-postgres. I'm using the following script as a test (new to node,js and wanted a really rough and ready test to compare it's performance against sinatra/rack).

var pg = require('pg');
var http = require('http');
var conString = "postgres://postgres:testing@127.0.0.1:5432/test";

pg.connect(conString, function(err,client){
http.createServer(function (req, res) {
res.writeHead(200, {'Content-Type': 'text/plain'});
client.query("select * from items", function(err, result) {
if (err)
{
console.log(err);
}
else
{
for(var i = 0;i<result.rows.length;i++)
{
res.write(result.rows[i].name);
}
}
res.end();
});
}).listen(1337, "127.0.0.1");
console.log('Server running at http://127.0.0.1:1337/');
});

One terminal has node running, and in a second I'm running the following: ab -n 20000 -c 5 http://127.0.0.1:1337/

The performance is amazing, but unfortunately about 30 seconds or so after the requests have all finished I get the following error

net.js:391
throw new Error('Socket is not writable');
^
Error: Socket is not writable
at Socket._writeOut (net.js:391:11)
at Socket.write (net.js:377:17)
at [object Object]._send (/home/richard/Documents/node_modules/pg/lib/connection.js:85:24)
at [object Object].end (/home/richard/Documents/node_modules/pg/lib/connection.js:186:8)
at [object Object].end (/home/richard/Documents/node_modules/pg/lib/client.js:158:19)
at Object.destroy (/home/richard/Documents/node_modules/pg/lib/index.js:43:16)
at Object.destroy (/home/richard/Documents/node_modules/pg/node_modules/generic-pool/lib/generic-pool.js:122:13)
at Object.removeIdle as _onTimeout
at Timer.callback (timers.js:83:39)

Can anyone help with the above - it's highly possibly that I've done something wrong, as I'm new to node, but feel that I've followed every instruction (nearly word for word).

Thanks

@polotek
Copy link

polotek commented Aug 30, 2011

I'm interested in this. A glance at the stack trace seems to suggest that the pool module is trying to destroy idle connections and at some point it runs across ones that have already been closed. pg tries to send a last packet to signal connection close, but if the socket has already been closed, that will fail and produce an error like this. That's just a guess though. If I get time today I'll try to reproduce this.

@booo
Copy link
Contributor

booo commented Aug 30, 2011

If you want to make use of connection pooling this should be the way to go:

(function() {
  var http, pg, s;
  pg = require("pg");
  http = require("http");
  s = "postgres://postgres:testing@localhost:5432/testing";
  (http.createServer(function(req, res) {
    return pg.connect(s, function(error, client) {
      if (error) {
        return console.log(error);
      } else {
        return client.query("SELECT NOW();", function(error, result) {
          if (error) {
            return console.log(error);
          } else {
            res.writeHead(200);
            res.write(result.rows);
            return res.end();
          }
        });
      }
    });
  })).listen(1337);
}).call(this);

@hiddenhippo
Copy link
Author

Many thanks for you help here. I've adjusted the connection string etc to
suit my environment and it works. I can now throw any number of requests at
the node.js instance and it doesn't crash. I was hoping that you could
explain why the approach below works and my original concept didn't. I've
only just started playing with event-driven programming, so conceptually I
don't see how your code below works and mine didn't. One of the main
differences that I can see is that originally I had the web instance within
the postgres function e.g.

pg.connect(conString, function(err,client){
http.createServer(function (req, res) {

Where as you take a different approach by placing the postgres function
within the http.

If you could explain why your code works over mine that'd be greatly
appreciated.

As a side note, your driver coupled with node.js (kind of the code below)
handles 40% more requests per second than a ruby sinatra instance with
sequel.

Thanks again.

On 30 August 2011 23:19, booo <
reply@reply.github.com>wrote:

If you want to make use of connection pooling this should be the way to go:

(function() {
 var http, pg, s;
 pg = require("pg");
 http = require("http");
 s = "postgres://postgres:testing@localhost:5432/testing";
 (http.createServer(function(req, res) {
   return pg.connect(s, function(error, client) {
     if (error) {
       return console.log(error);
     } else {
       return client.query("SELECT NOW();", function(error, result) {
         if (error) {
           return console.log(error);
         } else {
           res.writeHead(200);
           res.write(result.rows);
           return res.end();
         }
       });
     }
   });
 })).listen(1337);
}).call(this);

--
Reply to this email directly or view it on GitHub:
https://github.com/brianc/node-postgres/issues/45#issuecomment-1950289

@booo
Copy link
Contributor

booo commented Aug 31, 2011

I'm not 100% sure what's the problem is so I don't try to explain this.

What does the following statement do?

pg.connect(conString, function(error, client) {
  http.createServer(function(req, res) {
    client.query();
  }).listen(1337);
});

The pg.connect functions invokes the callback with the client as parameter on success. Then you create a http-server with a callback function which acts on the client returned from hte pg.connect function. The callback function provided to the createServer gets invoked for every http request. The createServer callback is in the scope of the pg.connect callback so on every request you share the same client. In your example you only use one client (connection) to query the database.

If you do it like I did you make use of the connection pooling implemented by node-postgres. For every http-request we try to get a client/connection from the pool. The default pool size is 10 so we make use of 10 "parallel" connections. Once all queries on a client are finished the "drain" event gets emited and the client is returned into the pool for us. After the client is released to the pool another http-request can make use of the client requesting it via pg.connect. The problem in your code maybe is that the client is released to the pool on the drain event but you try to reuse it later. Don't know... :)

Maybe take a look at the node-postgres source to understand the hole process flow (events).

@hiddenhippo
Copy link
Author

Thanks for your reply. Event driven coding requires a slight change of
"hat." With your explanation I see the error of my ways, in that by placing
the http function within the pg.connection function, the pg.connect client
is scoped through to the http-server, and therefore every request must use
this variable instance (just like you say). Where as having the pg.connect
function within the http function, every "request" has the ability to
"create" a new db connection - although naturally it could acquire one from
the pool, and therefore save a pretty heavy hit.

thanks again and I'll probably take a look at the code within node-postgres.

cheers

On 31 August 2011 20:22, booo <
reply@reply.github.com>wrote:

I'm not 100% sure what's the problem is so I don't try to explain this.

What does the following statement do?

pg.connect(conString, function(error, client) {
  http.createServer(function(req, res) {
    client.query();
 }).listen(1337);
});

The `pg.connect` functions invokes the callback with the client as
parameter on success. Then you create a http-server with a callback function
which acts on the client returned from hte `pg.connect` function. The
callback function provided to the `createServer` gets invoked for every http
request. The `createServer` callback is in the scope of the `pg.connect`
callback so on every request you share the same client. In your example you
only use one client (connection) to query the database.

If you do it like I did you make use of the connection pooling implemented
by node-postgres. For every http-request we try to get a client/connection
from the pool. The default pool size is 10 so we make use of 10 "parallel"
connections. Once all queries on a client are finished the "drain" event
gets emited and the client is returned into the pool for us. After the
client is released to the pool another http-request can make use of the
client requesting it via `pg.connect`. The problem in your code maybe is
that the client is released to the pool on the drain event but you try to
reuse it later. Don't know... :)

Maybe take a look at the node-postgres source to understand the hole
process flow (events).

--
Reply to this email directly or view it on GitHub:
https://github.com/brianc/node-postgres/issues/45#issuecomment-1960896

@brianc
Copy link
Owner

brianc commented Sep 2, 2011

@boo is exactly right with his follow up. Basically when you use the connection pool you are given a client with no pending queries in the client's internal query queue. Once you add a query to the previously idle client it goes right off and starts executing the first query. When the first query returns, if there are no more pending queries (that means you've not called client.query again before the first query's callback is finished) then the client reports itself as "drained" to the pool and is automatically returned to the pool.

In your example what is happening (to which @boo has already alluded) is you right out of the gate request an idle client from the pool. The pool happily complies and gives you a client. You then wait until your first http request to use this client...sending a single query to the idle client. The client happily dispatches the query and fires the callback as normal. Once the query is completed (assuming no second http request has come in since the first) the client is dispatched back to the pool and considered 'idle.' This is where problems start. Now you have a closure around the client which is now considered to be idle by the pool but is infact still being used in every http connection. The pool, after a configurable idle timeout, will close any clients it considers to be idle. So, you're basically using an idle client "secretly," and the pool at some point in the future shuts this client down...which is why you're getting a weird disconnect error some point in the future.

As a side note I've just pushed a new version of node-postgres which allows you to pause and resume the automatic draining of clients so you could technically now do this:

pg.connect(conString, function(err, client) {
  client.pauseDrain();
  http.createServer(function(req, res) {
    client.query('whatever');
    //go about your marry way
  });
});

because you called "pauseDrain" the client will never report itself as drained, the pool will never receive back the idle client, and your client connection will hopefully never be closed. My example above is a very bad example, because what it effectively does is completely removes the bennifit of client pooling and just gives your server one open client connection to postgres which could more easily be achieved like this:

  var client = new Client(conString);
  client.connect();
  http.createServer(function(req, res) {
    client.query('whatever');
    //go about your marry way
  });

tl;dr: you have a bug in the logic of your code. put the pg.connect call inside your http server request handler so you get a new connection on each http request.

@brianc brianc closed this as completed Sep 2, 2011
@hiddenhippo
Copy link
Author

thanks everyone for your comments, replies and guidance. greatly appreciated.

brianc added a commit that referenced this issue Dec 27, 2019
* Initial work

* Make progress on custom pool

* Make all original tests pass

* Fix test race

* Fix test when DNS is missing

* Test more error conditions

* Add test for byop

* Add BYOP tests for errors

* Add test for idle client error expunging

* Fix typo

* Replace var with const/let

* Remove var usage

* Fix linting

* Work on connection timeout

* Work on error condition tests

* Remove logging

* Add connection timeout

* Add idle timeout

* Test for returning to client to pool after error

fixes #48

* Add idleTimeout support to native client

* Add pg as peer dependency

fixes #45

* Rename properties

* Fix lint

* use strict

* Add draining to pool.end

* Ensure ending pools drain properly

* Remove yarn.lock

* Remove object-assign

* Remove node 8

* Remove closure for waiter construction

* Ensure client.connect is never sync

* Fix lint

* Change to es6 class

* Code cleanup & lint fixes
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

4 participants