Skip to content
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

PoolMetrics.allocatedSize reports a different allocation count when the allocator uses threads #172

Closed
mp911de opened this issue Jul 20, 2023 · 4 comments
Assignees
Labels
type/enhancement A general enhancement

Comments

@mp911de
Copy link
Collaborator

mp911de commented Jul 20, 2023

Expected Behavior

When using a threaded scheduler like Schedulers.parallel() or Schedulers.single(), the reported allocation size increments by one on a non-warmed pool. The pool allocates one element more than requested.

Leaving out subscribeOn(…) or using an immediate scheduler doesn't show the reported behavior.

Actual Behavior

The pool should not request additional items.

Steps to Reproduce

@Test
void reproCase() {
  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();

  assertThat(pool.metrics().allocatedSize()).isEqualTo(3);
}

Possible Solution

Your Environment

  • Reactor version(s) used: Reactor Pool 1.0.1, Reactor 3.6.0-M1
  • Other relevant libraries versions (eg. netty, ...):
  • JVM version (java -version):
  • OS and version (eg uname -a):
@pderop
Copy link
Contributor

pderop commented Jul 20, 2023

I applied the test on reactor-pool 1.0.0, and the test also fails, so it is not a regression from reactor-pool 1.0.1.

if my undertanding is correct, what happens is the following:

		Mono<Integer> allocator = Mono.just(1).subscribeOn(Schedulers.single());
		InstrumentedPool<Integer> pool = PoolBuilder.from(allocator).sizeBetween(3, 7).buildPool();
(1)		pool.acquire().block();
(2)		pool.acquire().block();
		assertThat(pool.metrics().allocatedSize()).isEqualTo(3);
  • the pool is created with min=3, max=7, and the allocator is subscribed to using Scheduler.single(), meaning that the allocations will complete asynchronously
  • when the first acquisition happens (1) from the junit thread, the lazy warmup is taking place: so a first resource is allocated, then two extra resources are also allocated. When the first allocation completes, it unblock (1), but the two other extra allocations are still pending (because the allocator is subscribed using Scheduler.single)
  • when (1) completes, (2) requests for a new resource, but the two extra resources are not yet completed, so an new unexpected resource is created for (2).
  • in the end, when the two extra resources are allocated, we end up with 4 created resources, which is unexpected.

@mp911de
Copy link
Collaborator Author

mp911de commented Jul 20, 2023

I'm not sure this is a bug per-se, it is mostly unexpected behavior and I'm not sure someone ever defined how much should be allocated.

@pderop
Copy link
Contributor

pderop commented Jul 20, 2023

I'm also unsure.
For the moment, I just have provided my analysis, and I need to discuss with the reactor team about this.

@pderop pderop added type/enhancement A general enhancement and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet labels Aug 22, 2023
@pderop pderop added this to the 1.0.2 milestone Aug 22, 2023
@pderop
Copy link
Contributor

pderop commented Sep 5, 2023

PR #173 for this issue was started (it's not yet finalized), however after more discussions with the team, it appears that, according to the javadoc of PoolBuilder.sizeBetween, It is not said that the resources must be kept to the minimum, it says that no more than max resources are created, and only best effort is done in order to maintain the minimum number of live resources to keep in the pool:

	 * @param min the minimum number of live resources to keep in the pool (can be best effort)

Moreover, the PR first waits for min resources to be created before allocating more resources, it means that
if we have say 1000 requests to acquire, with min=100, then it means that we will delay allocations of 900 resources until 100 resources are allocated (because min=100), so, warmup will not be enough, and it will delay for nothing because we actually need to allocate to the maximum

So, we think that this the current behavior is correct based on the javadoc.
I'm closing this issue, thanks.

@pderop pderop closed this as completed Sep 5, 2023
@pderop pderop removed this from the 1.0.2 milestone Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants