Skip to content

Revert "Count pending connects when considering max pool size" #550

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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

Revert "Count pending connects when considering max pool size" #550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 14 additions & 26 deletions src/internal/pool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ class Pool {
this._maxSize = config.maxSize
this._acquisitionTimeout = config.acquisitionTimeout
this._pools = {}
this._pendingCreates = {}
this._acquireRequests = {}
this._activeResourceCounts = {}
this._release = this._release.bind(this)
Expand All @@ -74,11 +73,18 @@ class Pool {
const key = address.asKey()

if (resource) {
resourceAcquired(key, this._activeResourceCounts)
if (this._log.isDebugEnabled()) {
this._log.debug(`${resource} acquired from the pool ${key}`)
if (
this._maxSize &&
this.activeResourceCount(address) >= this._maxSize
) {
this._destroy(resource)
} else {
resourceAcquired(key, this._activeResourceCounts)
if (this._log.isDebugEnabled()) {
this._log.debug(`${resource} acquired from the pool ${key}`)
}
return resource
}
return resource
}

// We're out of resources and will try to acquire later on when an existing resource is released.
Expand Down Expand Up @@ -179,7 +185,6 @@ class Pool {
if (!pool) {
pool = []
this._pools[key] = pool
this._pendingCreates[key] = 0
}
while (pool.length) {
const resource = pool.pop()
Expand All @@ -196,29 +201,12 @@ class Pool {
}
}

// Ensure requested max pool size
if (this._maxSize > 0) {
// Include pending creates when checking pool size since these probably will add
// to the number when fulfilled.
const numConnections =
this.activeResourceCount(address) + this._pendingCreates[key]
if (numConnections >= this._maxSize) {
// Will put this request in queue instead since the pool is full
return null
}
if (this._maxSize && this.activeResourceCount(address) >= this._maxSize) {
return null
}

// there exist no idle valid resources, create a new one for acquisition
// Keep track of how many pending creates there are to avoid making too many connections.
this._pendingCreates[key] = this._pendingCreates[key] + 1
let resource
try {
// Invoke callback that creates actual connection
resource = await this._create(address, this._release)
} finally {
this._pendingCreates[key] = this._pendingCreates[key] - 1
}
return resource
return await this._create(address, this._release)
}

async _release (address, resource) {
Expand Down
51 changes: 1 addition & 50 deletions test/internal/pool.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,59 +562,10 @@ describe('#unit Pool', async () => {
expectNumberOfAcquisitionRequests(pool, address, 0)
})

const address = ServerAddress.fromUrl('bolt://localhost:7687')

it('should consider pending connects when evaluating max pool size', async () => {
const conns = []
const pool = new Pool({
// Hook into connection creation to track when and what connections that are
// created.
create: (server, release) => {
// Create a fake connection that makes it possible control when it's connected
// and released from the outer scope.
const conn = {
server: server,
release: release
}
const promise = new Promise((resolve, reject) => {
conn.resolve = resolve
conn.reject = reject
})
// Put the connection in a list in outer scope even though there only should be
// one when the test is succeeding.
conns.push(conn)
return promise
},
// Setup pool to only allow one connection
config: new PoolConfig(1, 100000)
})

// Make the first request for a connection, this will be hanging waiting for the
// connect promise to be resolved.
const req1 = pool.acquire(address)
expect(conns.length).toEqual(1)

// Make another request to the same server, this should not try to acquire another
// connection since the pool will be full when the connection for the first request
// is resolved.
const req2 = pool.acquire(address)
expect(conns.length).toEqual(1)

// Let's fulfill the connect promise belonging to the first request.
conns[0].resolve(conns[0])
await expectAsync(req1).toBeResolved()

// Release the connection, it should be picked up by the second request.
conns[0].release(address, conns[0])
await expectAsync(req2).toBeResolved()

// Just to make sure that there hasn't been any new connection.
expect(conns.length).toEqual(1)
})

it('should not time out if max pool size is not set', async () => {
let counter = 0

const address = ServerAddress.fromUrl('bolt://localhost:7687')
const pool = new Pool({
create: (server, release) =>
Promise.resolve(new Resource(server, counter++, release)),
Expand Down