Too much resources may be created when using lazy-warmup

[pderop]

When the reactor-pool is configured with the sizeBetween method using min/max values, more resources than expected may be created.

For example, the following code will create 4 resources instead of the expected 3:

		Mono<Integer> allocator = Mono.just(1).subscribeOn(Schedulers.single());
		InstrumentedPool<Integer> pool = PoolBuilder.from(allocator).sizeBetween(3, 7).buildPool();
		pool.acquire().block();
		pool.acquire().block();

This is not a bug per se, but we can arrange to only create minimal necessary resources (3 in the above case).
The extra resource is created because when the second acquire takes place, the allocation for the two extra resources that are lazily created after the first acquire is still in progress and not yet completed, because the allocator is subscribed asynchronously using .subscribeOn(Schedulers.single())

As a result, we end up with four created resources, while we would expect to only have three resources:

• first resource created by the first acquire
• second and third resources created asynchronously due to lazy warmup triggered by the first acquire
• An unexpected fourth resource is created by the second acquire because the allocation of the second and third resources is still in progress at this point.

Fixes #172

[chemicL]

My feedback is in the comments. I think I understand the justification for the need to not allocate more than was demanded, but I'm afraid the provided solution will not prevent excessive warmup in case of high concurrency. It would work when concurrency is limited I believe. And perhaps in a highly concurrent scenario we'd actually want to allocate more. That's why I'm on the fence and just commenting instead of approving/requesting changes.

[chemicL]

Would it make sense to prevent the drainLoop() being called concurrently instead? Let's say that > 1 concurrent acquires happen. Both increment the demand and add themselves to the Borrower queue. However, just one enters the drainLoop and delivers all necessary resources. A loop can check if more demand has been added before it exits in order to repeat the procedure and ensure every acquire is satisfied in the end. But that would guarantee no simultaneous warmup happens.

[chemicL]

Just to clarify why drainLoop can be executed concurrently: both evictInBackground and pendingOffer methods have a check like this:

			if (WIP.decrementAndGet(this) > 0) {
				drainLoop();
			}

And it doesn't exclude other threads from entering.

[pderop]

if I'm correct drainLoop can't be run concurrently, it is protected using the WIP design pattern.
However, I will need to give more thinking about your comment, thanks.

[chemicL]

This is not a typical WIP pattern that is in use here. The above condition should be as the one in the drain method:

void drain() {
	if (WIP.getAndIncrement(this) == 0) {
		drainLoop();
	}
}

but it's not the case for evictInBackground and pendingOffer.

[chemicL]

This flag does not guarantee exclusivity IMO. A stress test could be added to validate my accusation and I worry it would hold true :(

[violetagg]

I also think so

[pderop]

I do not see for the moment the concurrency issue, but I will investigate this, and will create a stress test.
The flag is only set from the drainLoop and then it is reset once the subscription to the Flux.range completes, and if drainLoop is missing the update of warmupInProgress, then drain() will cause drainLoop to be called again.
However, since both of you are seeing an issue here, I'll carefully revisit all this.

[violetagg]

On error warmupInProgress will continue to stay true. Is that intentional?

[pderop]

good catch, the #175 is partially addressing this problem.

[pderop]

warmpupInProgress should be indeed reset even if there is an error. will think about this.

[pderop]

closing this PR, see #172.