AsyncObjectPool Tests + Fixes for Returning to Wrong Pool, Multiple Close, Missed Destruction

[SattaiLanfear]

Added a check to SingleThreadedAsyncObjectPool to disallow giveBack on an object that did not come from that pool.

[mauricio]

@SattaiLanfear can you include a spec showing the broken behavior so it doesn't get re-introduced?

[SattaiLanfear]

@mauricio Sure, I'll try to do that in the next few days, otherwise might take a couple weeks.

[mauricio]

Sure, not a problem!

Maurício Linhares http://mauricio.github.io/ - http://twitter.com/#!/mauriciojr

On Sat, Mar 5, 2016 at 7:03 PM, Stephen Couchman notifications@github.com wrote:

@mauricio https://github.com/mauricio Sure, I'll try to do that in the next few days, otherwise might take a couple weeks.

— Reply to this email directly or view it on GitHub https://github.com/mauricio/postgresql-async/pull/167#issuecomment-192763103 .

[SattaiLanfear]

@mauricio Working on the spec and looking at the SingleThreadedAsyncObjectPool implementation left me with a question.

In giveBack, we fail the Promise if the object being returned fails validation, but as the only actions to be taken seem to be internal (dropping the item out of the pool and instructing the factory to destroy it) that doesn't seem a desirable outcome. From an external view, the item was returned successfully and the pool subsequently decided to destroy it.

Can you provide any insight into why we chose this behavior, particularly as the failure is often discarded by most useage patterns, such as pool.use?

[mauricio]

@SattaiLanfear I don't think there was any thinking other than if something fails let's bubble it up, we could definitely not break the promise but we'd still need to report this error in some way so that it doesn't break in silence.

[SattaiLanfear]

@mauricio Followup question, since I decided if I was going to write a spec I might as well make it as complete as I could, I've been examining the expected behavior of the rest of AsyncObjectPool and in the process failed one of the tests I'd written.

In SingleThreadedAsyncObjectPool we have:

  private def testObjects {
    val removals = new ArrayBuffer[PoolableHolder[T]]()
    this.poolables.foreach {
      poolable =>
        this.factory.test(poolable.item) match {
          case Success(item) => {
            if (poolable.timeElapsed > configuration.maxIdle) {
              log.debug("Connection was idle for {}, maxIdle is {}, removing it", poolable.timeElapsed, configuration.maxIdle)
              removals += poolable
              factory.destroy(poolable.item)
            }
          }
          case Failure(e) => {
            log.error("Failed to validate object", e)
            removals += poolable
          }
        }
    }
    this.poolables = this.poolables.diff(removals)
  }

If the item fails validation it doesn't get passed to the factory for destruction, just dropped from the pool, is that intentional?

[mauricio]

Nope, should be sent to destruction in this case as well.

[SattaiLanfear]

@mauricio I've updated the pull request's title to better fit what's in the pull request now.

New Summary: Added a generic spec for anything claiming to be a AsyncObjectPool. Created a specialized spec for SingleThreadedAsyncObjectPool. Fixed allowing items to be returned that didn't come from this pool. Fixed not destroying items that failed in-pool validation. Fixed exception on multiple close attempts to make consistent despite race conditions.