Skip to content

if server is unavail connection hangs over other connection #31

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

Closed
mitechie opened this issue May 26, 2011 · 11 comments
Closed

if server is unavail connection hangs over other connection #31

mitechie opened this issue May 26, 2011 · 11 comments
Labels

Comments

@mitechie
Copy link

I'm working on error catching a postgres condition when the server is not available. The first time, everything works fine. The error is passed in and I can check for it and I'm set.

The second pass in hangs the at the connection line and I never get to the callback to get the error.

    console.log('about to connect');
    pg.connect(DBSTRING, function(err, client) {
        if (err) {
            console.log("ERROR DB " + err.message);
            stream.write(err.message);
        } else {
            // do query and such
        }
    });

Is the bit of code I'm working with. The about to connect is dumped on every request in, but the callback only seems to occur every other pass into here. Am I missing something in the error handling? Is there something I can check?

Thanks

@brianc
Copy link
Owner

brianc commented May 26, 2011

Would you be able to create a gist or test case which I can execute to reproduce the issue?

@mitechie
Copy link
Author

Sure thing, this is a gist with the two files cut down as much as I can get. Basically there's a http app that calls the postgres app. I've left in the debugging message I was using and the top of the file walks through where I'm getting the issue.

Let me know if I can help in any way.

https://gist.github.com/993332

Thanks

Rick

@brianc
Copy link
Owner

brianc commented May 26, 2011

Thank Rick. I hate to be a pain but do you think you could reproduce the issue in a more isolated way? Like just a script that maybe calls pg.connect a few times with the failing connections? Also, what type of connection error is being returned when you do receive the error?

Remove any http server or socket things if possible. I'll work with what you've given to try to reproduce later this evening, but the more variables we can remove from the equation the easier it will be to identify the problem within node-postgres.

@mitechie
Copy link
Author

OK, this is a smaller gist of just the pgconnect. When I test it, I only get the error message every other call to pgconnect:

https://gist.github.com/993332

Sample output:

running
interval set

calling
    in check
    connect
    ERROR DB ECONNREFUSED, Connection refused
calling
    in check
calling
    in check
    connect
    ERROR DB ECONNREFUSED, Connection refused
calling
    in check
calling
    in check
    connect
    ERROR DB ECONNREFUSED, Connection refused

@brianc
Copy link
Owner

brianc commented May 26, 2011

Perfect! Will fix this after work today! <3

@brianc
Copy link
Owner

brianc commented Jun 7, 2011

I figured out this problem & have written a failing test for the scenario, unfortunately it's a bit tricky. I've been on holiday and so haven't had much time to work on it...I'll push the failing test out tonight & go over what's going on.

@mitechie
Copy link
Author

mitechie commented Jun 7, 2011

Thanks for the update. Fortunately it's not killer since the db is up and I'm just testing node.js for a use case right now.

@tmuellerleile
Copy link

Hi, is there any progress on this issue? As far as I can see the problem can be traced back to the following lines in Connection.connect

  if(this.stream.readyState === 'closed'){
    this.stream.connect(port, host);
  }
  else if(this.stream.readyState == 'open') {
    this.emit('connect');
  }

As the underlying stream is in state opening after the first connection attempt, no action is actually being taken here on the second one. Repeating the connection attempt only leads to adding another client to the connection pool (until all its free slots are exhausted).

My first attempt at a solution is adding a line in pg.connect:

var onError = function(error) {
  client.removeListener('connect', onReady);
  callback(error);
  client = new Client(config); // <- this one
  pool.checkIn(client);
}

This seems to work fine here, however I am not convinced that this is actually the best solution as it leaves the original stream object (and underlying fd) in its half-open state.

@freewil
Copy link

freewil commented Sep 19, 2011

@brianc Any progress with this? I'm running into a similar scenario where I want to be able to handle disconnects/connection failures from the server and try to reconnect on a given interval.

@hampsterx
Copy link

bump this is kind of a huge deal breaker for me right now!! Restart postgres and my nodes apps are hung.. :(

@brianc
Copy link
Owner

brianc commented Nov 7, 2012

Do you have a way to reproduce this? A failing test? It's been a long while since I've taken a look at this. If you can gist some code to reproduce the issue I can absolutely fix it within 1-2 days.

@brianc brianc closed this as completed Jan 25, 2013
brianc pushed a commit that referenced this issue Dec 27, 2019
* Revert "When connection fail, emit the error. (#28)"

This reverts commit 6a7edab.

The callback passed to `Pool.prototype.connect` should be responsible for handling connection errors. The `error` event is documented to be:

> Emitted whenever an idle client in the pool encounters an error.

This isn’t the case of an idle client in the pool; it never makes it into the pool.

It also breaks tests on pg’s master because of nonspecific dependencies.

* Don’t create promises when callbacks are provided

It’s incorrect to do so. One consequence is that a rejected promise will be unhandled, which is currently annoying, but also dangerous in the future:

> DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

The way callbacks are used currently also causes #24 (hiding of errors thrown synchronously from the callback). One fix for that would be to call them asynchronously from inside the `new Promise()` executor:

    process.nextTick(cb, error);

I don’t think it’s worth implementing, though, since it would still be backwards-incompatible – just less obvious about it.

Also fixes a bug where the `Pool.prototype.connect` callback would be called twice if there was an error.

* Use Node-0.10-compatible `process.nextTick`
brianc pushed a commit that referenced this issue Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants