2.0 #67

brianc · 2017-07-09T02:15:39Z

This contains a massive number of internal changes. However, the external API remains very close to the same.

The breaking changes are:

Drop support for node < 4.x. closes drop support of node <4 ? #56
No longer use node-pool as a dependency so any configuration options you previously passed to node-pool will not work anymore except for max and idleTimeoutMillis
Cannot call pool constructor without new keyword

I attempted to upgrade to node-pool@3.x, but it weirdly no longer supports returning an error when a client fails to connect. Instead it tries to reconnect the client in a tight loop and makes the error non-actionable. I was unable to find a good way to hook into this behavior and override it. Furthermore it has a lot of additional bells and whistles for priority queuing, min levels, auto-connection, and other behavior pg and pg-pool have never used. Initially I was distraught at the thought of implementing the pooling behavior here, but it turns out it wasn't very much code & all the tests still pass!

After this I spent some time going through and writing tests for many of the open issues to verify they no longer happen. I also added support for the long requested connectionTimeoutMillis - now supplying connectionTimeoutMillis as a millisecond value will cause the pool to return an error on your connect call if a connection could not be established within the configured time. By default there is no timeout.

Known fixes:

Destroyed workers are always recreated without the previous workarounds for node-pool
pg>=5.0 is a peerDependency. closes pg dependency #45
added .ending property which will allow you to inspect when the pool is in the process of shutting down. closes Fails to reconnect after invoking end() #58
ensure idleTimeoutMillis of 0 disables idle timeout. closes Does idleTimeoutMillis = -1 keep connections open indefinently #64

fixes #48

fixes #45

calebboyd

Neat! I'm excited to see that the pool is being dropped.

I did notice that promise support is still a bit second class. In that -- promisified apis would be able to perform much better than the scheme currently used.

calebboyd · 2017-07-10T04:27:32Z

index.js

-    this.emit('error', e, client)
-  }.bind(this))
+function release (client, err) {
+  client.release = function () { throw new Error('called release twice') }


This function could be hoisted

good call. done.

calebboyd · 2017-07-10T04:44:58Z

index.js

+  this._pulseQueue()
 }

 Pool.prototype.connect = function (cb) {


It looks like the callback in the new version may be unintentionally synchronous where the old version was async

Is that bad? It's a semver major bump & I'd rather not introduce a nextTick just to keep it async.

calebboyd · 2017-07-10T04:57:37Z

test/bring-your-own-promise.js

+
+const describe = require('mocha').describe
+const it = require('mocha').it
+const BluebirdPromise = require('bluebird')


BluebirdPromise.coroutine == co.wrap if you want to drop a dep

nice! today I learned!

calebboyd · 2017-07-10T05:12:56Z

index.js

-          reject(err)
-        }
-        return
+        return cb(err, undefined, function () { })


I think this block is unreachable based on the use of _pendingQueue

calebboyd · 2017-07-10T05:18:16Z

index.js

+  this.log('ending')
+  if (this.ending) {
+    const err = new Error('Called end on pool more than once')
+    return cb ? cb(err) : this.Promise.reject(err)


sync : async

Is it bad to do sync if its a callback and async if it's a promise? Promises are always async, but callbacks done necessarily have to be and AFAIK there's never been an idiom saying they should be.

In general, Its not good to mix async and sync in the same API. Whats weird here is nothing invoked internally is actually async except the result. In one case I can get the error synchronously (looks async) and the other, async.

If I were to express a preference it would be to stick with an async api throughout.

Here is a tree of posts that go into a lot more detail http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony

Good point - I'll fix this

calebboyd · 2017-07-10T05:18:48Z

index.js

+  return promised.result
 }
+
+Object.defineProperty(Pool.prototype, 'waitingCount', {


Could clean this up a lot if the export was switched to something like:

module.exports = function (options, client) { return new PgPool(options, client) }

I'm not following how this would be cleaned up? Could you expand on what you meant here?

np, I just mean taking advantage of the class sugar for function defs and getters of the new min node version =)

class PgPool { _isFull() {..} ... connect() {...} ... get waitingCount() {...} get totalCount() {...} get idleCount() {...} } module.exports = function (options, client) { return new PgPool(options, client) } ```

I'll try that & see if it works in node 4. If so, definitely will go this way.

calebboyd · 2017-07-10T05:28:12Z

index.js

+    const response = this._promisify(cb)
+    const result = response.result
+    cb = response.callback
+    this._pendingQueue.push((err, client) => {


proposal: instead of handling this in a closure -- just store the waiter and handle this when invoking it during dequeue/shift

excellent suggestion! cleaned this up.

abenhamdine · 2017-07-10T10:41:36Z

.travis.yml

    - node_js: "6"
      addons:
        postgresql: "9.4"
-    - node_js: "8"


Just wonder : why remove node 8 ?

it was messing up in travis but working locally even though the same tests were passing locally - I just removed it temporarily so I could make sure the other tests were passing. I'll need to get it working in travis eventually.

brianc · 2017-07-10T14:40:47Z

Thank you so much for the feedback and review! I'm not quite done w/ this branch yet - code's still a bit dirty...but have quite a bit of client work in the next few days so I'll take me a bit of time to get to this. But know I'm grateful & I'll be back within a day or two! 💃

brianc · 2017-07-11T22:25:39Z

I did notice that promise support is still a bit second class. In that -- promisified apis would be able to perform much better than the scheme currently used.

@calebboyd - using callbacks internally in this module is so we can support pg>=5.x instead of pg@7 which is when all the APIs for pg are fully promisified. What do you mean by that exactly - could you elaborate?

calebboyd · 2017-07-12T04:14:21Z

Mostly I think that it would be best to stick with one API or the other.
My reasoning is that: accepting a custom Promise at this level would cut into performance.

For example, Bluebird.promisify, will rewrite the function (emit new code) in order to maintain low overhead when allocating new Promises -- and then uses an internal implementation. Node's require('util').promisify will use a native deferred api. Each of which would behave much faster than creating the new Promise and deferred mechanism being used. As a plus, plain callbacks for waiters in this internal queue will always be the quicker solution of the two.

In general sticking with callbacks here will allow people who use promises (I'm one) to do so with the lowest overhead, which is a plus for the library that proxies queries.

calebboyd · 2017-07-12T04:30:12Z

I guess that wasn't super clear. I don't have anything that would stop me from using this (I could promisify it anyways to avoid the issue). But Maybe the removal of this.Promise such that promisify is encouraged instead. As that is generally blessed now (in node core)

brianc · 2017-07-12T15:16:27Z

I'll take a look at hard look at the code once I finish it to ensure it's not doing unnecessary work wrt allocating promises. It should only allocate a promise if you don't supply a callback: if you use a callback it will never even allocate any promises so you don't have to be concerned about any performance as some folks who use callbacks tend to be. 🙅

As far as Bluebird.promsify versus the ability to accept a callback or return a promise (the api that this lib is going for) - I'd be pretty surprised if Bluebird.promsify generates any noticeable performance gains over what's going on in this code, but I'll investigate that to see...and like you said if someone prefers that approach nothing is stopping them from doing that as well in their own code on top of the pool. 👍

The goal is to support both promises and callbacks in a transparent way versus requiring someone run Bluebird.promsify or something similar on the pool, at least for the time being: supply a callback & callbacks will be used. Don't & you'll get a promise back. 😄

Thanks again for the feedback & comments. They're very helpful! 👏

calebboyd · 2017-07-12T15:57:43Z

Thank you for entertaining them. I'm very grateful for your work on these tools 🔨

brianc added 25 commits June 30, 2017 17:00

Initial work

e2bf705

Make progress on custom pool

d3c86fe

Make all original tests pass

0f6e85d

Fix test race

3cc72ce

Fix test when DNS is missing

1581df3

Test more error conditions

2205dd8

Add test for byop

3d8a9da

Add BYOP tests for errors

a3de64e

Add test for idle client error expunging

7d6a189

Fix typo

a85e9ec

Replace var with const/let

c970076

Remove var usage

0b421c0

Fix linting

20c4c34

Work on connection timeout

cb7b8e6

Work on error condition tests

8b9e1fb

Remove logging

93dcadb

Add connection timeout

4d5bac8

Add idle timeout

947d7a8

Test for returning to client to pool after error

87d657d

fixes #48

Add idleTimeout support to native client

7d9b22a

Add pg as peer dependency

2085a6d

fixes #45

Rename properties

f356bae

Fix lint

e1b6ea9

use strict

be8d58b

Add draining to pool.end

36f1c9a

brianc mentioned this pull request Jul 9, 2017

Upgrade generic-pool to v3 brianc/node-postgres#1308

Closed

brianc added 4 commits July 9, 2017 01:30

Ensure ending pools drain properly

bfd4f37

Remove yarn.lock

4af9ad4

Remove object-assign

bfa7ae7

Remove node 8

024795e

calebboyd reviewed Jul 10, 2017

View reviewed changes

abenhamdine reviewed Jul 10, 2017

View reviewed changes

Remove closure for waiter construction

6a2a302

brianc added 4 commits July 13, 2017 12:53

Ensure client.connect is never sync

0692b96

Fix lint

e444519

Change to es6 class

fc6c2de

Code cleanup & lint fixes

cbdbccc

brianc merged commit a0eb36d into master Jul 14, 2017

brianc mentioned this pull request Jul 15, 2017

Adding support for isDestroyed function #59

Closed

sehrope mentioned this pull request Aug 8, 2018

Problems with config.min #104

Closed

2.0 #67

2.0 #67

Uh oh!

Conversation

brianc commented Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calebboyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianc commented Jul 10, 2017

Uh oh!

brianc commented Jul 11, 2017

Uh oh!

calebboyd commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calebboyd commented Jul 12, 2017

Uh oh!

brianc commented Jul 12, 2017

Uh oh!

calebboyd commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brianc commented Jul 9, 2017 •

edited

Loading

calebboyd commented Jul 12, 2017 •

edited

Loading