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.

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

Jump to bottom

Warmup procedure fails if warmups were found #180

Closed
kprzybylo opened this issue Dec 21, 2023 · 2 comments · Fixed by #181
Closed

Warmup procedure fails if warmups were found #180

kprzybylo opened this issue Dec 21, 2023 · 2 comments · Fixed by #181
Assignees
@pderop
Labels
type/bug A general bug warn/regression A regression from a previous release
Milestone
1.0.5

Comments

@kprzybylo
Copy link

Expected Behavior

When a warmup method is run, but no warmups to run were found it should not do anything.

Actual Behavior

When a warmup method is run, but no warmups to run were found SimpleDequePool picks mergeConcurrency as lowest number of configured warmupParallelism and length of found warmups. Found warmups is 0, thus this values is passed further to Flux.merge and results in error from FluxFlatMap.
java.lang.IllegalArgumentException: maxConcurrency > 0 required but it was 0

Steps to Reproduce

We have configured scheduled warmup with our custom find out idle timeout that caused pool to increase our response time. Thus we are running warmup once per X second, but it's not always needed as in high traffic this just simply should be ignored. First run after app starts is fine as then maxConcurrency is 1, but later it's mostly 0 and we just have constant errors.

Possible Solution

Add check before executing warmups if there is anything to run to avoid unnecessary call for warmup with invalid configuration.

Your Environment

Spring Boot 3 App with Netty server and reactor-pool 1.0.3.

  • Reactor version(s) used: 1.0.3
  • Other relevant libraries versions (eg. netty, ...): netty, Spring Boot 3
  • JVM version (java -version): 17
@reactorbot reactorbot added the ❓need-triage This issue needs triage, hasn't been looked at by a team member yet label Dec 21, 2023
@pderop pderop self-assigned this Dec 21, 2023
@pderop pderop added status/need-investigation This needs more in-depth investigation and removed ❓need-triage This issue needs triage, hasn't been looked at by a team member yet labels Dec 21, 2023
@kprzybylo
Copy link
Author

Looking at branch 0.1.x seems it might be resolved, but I can't update reactor-pool to higher version to confirm that (parent library limitation). At this moment we are catching and ignoring this exception.

@pderop pderop added type/bug A general bug and removed status/need-investigation This needs more in-depth investigation labels Dec 21, 2023
@pderop pderop mentioned this issue Dec 21, 2023
Merged
@pderop
Copy link
Contributor

pderop commented Dec 21, 2023

@kprzybylo ,

thanks for reporting, this is a regression from #171, only for 1.0.x branch, originating from 1.0.1 version.
There is this ongoing PR which is addressing the problem: #181

@pderop pderop added this to the 1.0.5 milestone Dec 21, 2023
@pderop pderop added the warn/regression A regression from a previous release label Dec 21, 2023
pderop added a commit that referenced this issue Jan 4, 2024
@pderop
Warmup throws IllegalArgumentException in case no permits are availab…
3dae6b7 
…le (#181)

This fixes a regression that comes from #171, from 1.0.1 version: since the pool warmup method allows to be called at anytime, then if no more permits are available from the allocation strategy, the Flux.merge method is then called with a mergeConcurrencypameter that is set to 0, causing the following exception:

maxConcurrency > 0 required but it was 0
java.lang.IllegalArgumentException: maxConcurrency > 0 required but it was 0
	at reactor.core.publisher.FluxFlatMap.<init>(FluxFlatMap.java:74)
	at reactor.core.publisher.Flux.merge(Flux.java:1406)
	at reactor.core.publisher.Flux.merge(Flux.java:1375)
Added a check in warmup to immediately return in case no permits are available.
Also added a test case.

Fixes #180
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pderop pderop

Labels
type/bug A general bug warn/regression A regression from a previous release
Projects
None yet
Milestone
1.0.5
Development

Successfully merging a pull request may close this issue.

Warmup throws IllegalArgumentException in case no permits are available pderop/reactor-pool
3 participants
@pderop @reactorbot @kprzybylo