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

Fix bug in Retry AsyncContext's onResult consumeResultBaeforeRetryAttempt #2035

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Fix bug in Retry AsyncContext's onResult consumeResultBaeforeRetryAttempt #2035

merged 2 commits into from
Dec 20, 2023

Conversation

qnwlqnwlxm
Copy link
Contributor

Fix bug in Retry AsyncContext's onResult consumeResultBaeforeRetryAttempt.

injae-kim
injae-kim reviewed Oct 20, 2023
View reviewed changes
resilience4j-retry/src/main/java/io/github/resilience4j/retry/internal/RetryImpl.java
Comment on lines -363 to -366
if (attempt >= maxAttempts) {
if(consumeResultBeforeRetryAttempt != null){
consumeResultBeforeRetryAttempt.accept(attempt, result);
}
Copy link

@injae-kim injae-kim Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(FYI) On AsyncContextImpl, consumeResultBeforeRetryAttempt is only invoked when attempt >= maxAttempts and it seems bug!
I think it should be invoked on each retry.

On ContextImplt, consumeResultBeforeRetryAttempt is invoked on each retry well.

@qnwlqnwlxm
Clear Thread interrupted status in SupplierRetryTest
af15569 
Clears the interrupted status caused by the shouldMarkThreadInterruptedWhenInterruptedDuringRetry test method in SupplierRetryTest
@qnwlqnwlxm
Copy link
Contributor Author

@RobWin
Sorry. Instead of running the entire test, I only ran the new test I added.
So I couldn't confirm the issue. Now i'm fix it.

shouldMarkThreadInterruptedWhenInterruptedDuringRetry method of the SupplierRetryTest class interrupted the thread and did not clear it.
This was causing an InterruptedException in AsyncUtils.awaitResult when calling completionStage.toCompletableFuture().get(timeoutSeconds, TimeUnit.SECONDS).
The fix is to clear the interrupted status of the thread at the end of the shouldMarkThreadInterruptedWhenInterruptedDuringRetry test method.

cc. @injae-kim

@RobWin RobWin added this to the 2.2.1 milestone Dec 20, 2023
@RobWin RobWin merged commit 7588101 into resilience4j:master Dec 20, 2023
RobWin added a commit that referenced this pull request Jan 5, 2024
@hexmind @mikedias
@mdias-atlassian @hongbin-dev @hongbin-deali @goafabric @dependabot @tobi5775 @nowkarol @nluk @lukasz-nowak-idemia @msohaill @SOOHYUN-LIM @Lim-Soohyun @b923 @laviua @kerwin612 @mkopylec @deecodeme @tanuja5 @graemerocher @seokgoon28 @jattisha @Hartigan @ShijunDeng @qnwlqnwlxm @storozhukBM @Romeh @KrnSaurabh @RobWin @drmaas
Adaptive bulkhead 3 (#2085)
07a8304 
* Issue #201 duplication removed from PredicateCreator clients (#1921)

* Move toString() out of the builder class (#1913)

Co-authored-by: Mike Dias <mdias@atlassian.com>

* docs: remove deprecated http client (#1930)

Co-authored-by: hongbin-deali <hongbin.kim@deali.net>

* configuration for native image added (#1883)

Co-authored-by: Andreas Mautsch <amautsch>

* Bump actions/checkout from 3.2.0 to 3.4.0 (#1911)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3.2.0 to 3.4.0.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3.2.0...v3.4.0)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump actions/cache from 3.0.11 to 3.3.1 (#1907)

Bumps [actions/cache](https://github.com/actions/cache) from 3.0.11 to 3.3.1.
- [Release notes](https://github.com/actions/cache/releases)
- [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md)
- [Commits](actions/cache@v3.0.11...v3.3.1)

---
updated-dependencies:
- dependency-name: actions/cache
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Issue #1875: add copyright information to jar (#1876)

* add copyright information to jar

* add license file to jar

---------

Co-authored-by: TWI <audi-connect@msg.group>

* Add missing validation of config fields (#1931)

* Add missing validation of config fields

Document existing assumptions

* Fix range in exception message

* Mentioned removed deprecated properties in v2.0.0 changelog (#1954)

Co-authored-by: Łukasz Nowak <lukasz.nowak@idemia.com>

* Adding unchecked() method to CheckedConsumer (#1981)

Co-authored-by: Muhammad Sohail <muhammad.sohail@autodesk.com>

* FallbackMethod supports AOP (#1965)

* Add AOP in FallbackMethod

* Add test

---------

Co-authored-by: 임수현 <soohyun.lim2@cj.net>

* Prepare release 2.1.0

* Prepare release 2.1.0

* Updated version to 2.2.0-SNAPSHOT

* Issue #1761: Async retry doesn't emit event bugfix (#1986)

Co-authored-by: Hrbacek, David <david.hrbacek@firma.seznam.cz>

* Issue #1600: Metric for total number of invoked calls from retry (#1895)

* Issue #1600: Metric for total number of invoked calls from retry

* Issue #1600: Metric for total number of invoked calls from retry. Update

* Issue #1600: Metric for total number of invoked calls from retry. Update

* fix Feign fallback from lambda (#1999)

* fix Feign fallback from lambda

* Provide test cases for pr#1999

* Micrometer Timer decorator (#1989)

* Timer reactive support (#2009)

* Timer spring support (#2020)

* Discussion #1962: Added apache commons configuration based registries bootstraping (#1991)

* Removing stale retry configurations from configuration map #2037 (#2039)

* Bump org.jetbrains.kotlin.jvm from 1.7.22 to 1.9.0 (#1990)

Bumps [org.jetbrains.kotlin.jvm](https://github.com/JetBrains/kotlin) from 1.7.22 to 1.9.0.
- [Release notes](https://github.com/JetBrains/kotlin/releases)
- [Changelog](https://github.com/JetBrains/kotlin/blob/master/ChangeLog.md)
- [Commits](JetBrains/kotlin@v1.7.22...v1.9.0)

---
updated-dependencies:
- dependency-name: org.jetbrains.kotlin.jvm
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fixed Issue #2040: Micronaut BulkheadInterceptor always uses the "default" configuration in micronaut application

* Support Micronaut 4 (#1951)

* FallbackMethod supports AOP (#2058)

* Add AOP in FallbackMethod

* Add test

* Fixed the fallback method bug

---------

Co-authored-by: 임수현 <soohyun.lim2@cj.net>

* Support class name using SpEL expression at Circuitbreaker annotation name field. (#2053)

* Issue #1565: Do not retry if IntervalFunction returns interval less than 0. Emit correct events based on result of IF. (#2067)

* Fixed time-based tests

* Updated version to 2.2.0

* Do not interrupt future if running (#2072)

* Avoid to add  duplicate consumer (#2074)

* Updated version to 2.3.0-SNAPSHOT

* Improved EventProcessorTest

* Fix bug in Retry AsyncContext's onResult consumeResultBaeforeRetryAttempt (#2035)

* Adaptive bulkhead 2023.1.4 (#1873)

* Super early draft of adaptive bulkhead.
!!! NOT TESTED !!!

* Small proof of concept test.

* Code cleanup. Builder for BulkheadAdaptationConfig

* Small code clean up. Metrics implementation. Additional TODOs

* Additional TODOs

* Small renaming

* first round of adaptive bulkhead updates

* first round of adaptive bulkhead updates

* first round of adaptive bulkhead updates

* second round of adaptive bulkhead updates

* Sonar fixes

* adaptive bulkhead refactoring to match last discussion

* adaptive bulkhead refactoring to match last discussion

* adaptive bulkhead refactoring to match last discussion

* adaptive bulkhead refactoring to match last discussion

* adaptive bulkhead refactoring to match last discussion

* adaptive bulkhead refactoring to match last discussion

* Sonar fixes

* reduce code duplication

* add exception predicate unit testing

* first round of updates after introducing sliding window with AIMD limiter

* Sonar fixes

* Sonar fixes

* first round of updates for making adaptive policy independent from bulkhead

* first round of applying the review comments

* first round of applying the review comments

* second round of applying the review comments

* Third round of applying the review comments

* fix javadoc

* sonar fixes

* review comments round 4

* code cleanup

* review comments round 5

* review comments round 5

* review comments round 5

* #review comments

* Sonar fixes

* increase test coverage

* first round of review comments apply

* first round of review comments apply

* fix compile issue

* tune the adaptive bulkhead test

* New Draft

* Adaptive Bulkhead Draft

* Adaptive Bulkhead 2 Draft (#1306)

* Remove AIMD leftovers (#1397)

* Issue#651: Support to exclude MetricsAutoConfiguration

* Propagate clock into CircuitBreakerMetrics to allow mocked time-based tests. (#844)

* Issue #596: The Spring Boot fallback method is not invoked when a BulkheadFullException occurs #847

The ThreadPoolBulkhead is does not return a CompletionStage when a task could not be submitted, because the Bulkhead is full. The ThreadPoolBulkhead throws a BulkheadFullException instead. Analog to the ThreadPoolExecutor which throws the RejectedExecutionException. The BulkheadFullException is not handled correctly by the BulkheadAspect.
The BulkheadAspect should convert the BulkheadFullException into a exceptionally completed future so that the FallbackDecorator works as expected.

* Added timelimiter support for resilience4j-ratpack (#865)

* Sonar critical issues solved: (#876)

* Remove usage of generic wildcard type.
* Define a constant instead of duplicating this literal
* Refactor this method to reduce its Cognitive Complexity
* Add a nested comment explaining why this method is empty
* Class names should not shadow interfaces or superclasses
Warnings removed.

* Updated Gradle, Migrated from Bintray to Sonatype, migrated to GitHub Actions

Co-authored-by: Robert Winkler <rwinkler@telekom.de>

* Fixed sonar smells

* Small proof of concept test.

* first round of adaptive bulkhead updates

* second round of adaptive bulkhead updates

* Sonar fixes

* first round of applying the review comments

* Sonar fixes

* first round of review comments apply

* tune the adaptive bulkhead test

* New Draft

* Adaptive Bulkhead 2 Draft (#1306)

* Remove AIMD leftovers (#1397)

* updated to master

---------

Co-authored-by: bstorozhuk <storozhuk.b.m@gmail.com>
Co-authored-by: Mahmoud Romeh <mahmoud.romeh@collibra.com>
Co-authored-by: Robert Winkler <rwinkler@telekom.de>
Co-authored-by: KrnSaurabh <39181662+KrnSaurabh@users.noreply.github.com>
Co-authored-by: Robert Winkler <robwin@t-online.de>
Co-authored-by: Dan Maas <daniel.maas@target.com>

* Issue #201 bulkhead events corrected

* Issue #201 BulkheadOnLimitIncreasedEvent, BulkheadOnLimitDecreasedEvent merged into one event

* Issue #201 adaptive bulkhead config and time fixed

* Adaptive bulkhead 4.1 (#1947)

* Issue #201 EventPublisher hierarchy fixed

* Issue #201 onResult added to AdaptiveBulkhead

* Issue #201 states extracted to own files

* Issue #201 AdaptiveBulkhead states simplified (#1995)

* Issue #201 IncreaseInterval for SlowStartState added (#2001)

* Issue #201 timeUnit replaced by currentTimestamp (#2007)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Mike Dias <mike.rodrigues.dias@gmail.com>
Co-authored-by: Mike Dias <mdias@atlassian.com>
Co-authored-by: Hongbin Kim <fusis1@naver.com>
Co-authored-by: hongbin-deali <hongbin.kim@deali.net>
Co-authored-by: Andreas Mautsch <amautsch@gmx.de>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: tobi5775 <50146675+tobi5775@users.noreply.github.com>
Co-authored-by: TWI <audi-connect@msg.group>
Co-authored-by: Karol Nowak <nowkarol+github@gmail.com>
Co-authored-by: Łukasz Nowak <38426907+nluk@users.noreply.github.com>
Co-authored-by: Łukasz Nowak <lukasz.nowak@idemia.com>
Co-authored-by: Muhammad Sohail <mhsohail56@gmail.com>
Co-authored-by: Muhammad Sohail <muhammad.sohail@autodesk.com>
Co-authored-by: SOOHYUN-LIM <sh.lim7682@gmail.com>
Co-authored-by: 임수현 <soohyun.lim2@cj.net>
Co-authored-by: Robert Winkler <rwinkler@telekom.de>
Co-authored-by: David Hrbacek <35535320+b923@users.noreply.github.com>
Co-authored-by: Hrbacek, David <david.hrbacek@firma.seznam.cz>
Co-authored-by: Oleksandr L <laviua@users.noreply.github.com>
Co-authored-by: Kerwin Bryant <kerwin612@qq.com>
Co-authored-by: Mariusz Kopylec <mariusz.kopylec@o2.pl>
Co-authored-by: Mariusz Kopylec <mariusz.kopylec@allegro.pl>
Co-authored-by: Deepak Kumar <deep.rnj@gmail.com>
Co-authored-by: tanuja5 <1997.tanuja@gmail.com>
Co-authored-by: Graeme Rocher <graeme.rocher@gmail.com>
Co-authored-by: seokgoon28 <148044191+seokgoon28@users.noreply.github.com>
Co-authored-by: jattisha <jovanattisha@yahoo.com>
Co-authored-by: Hartigan <hartigans@live.com>
Co-authored-by: shijun_deng <dengshijun1992@qq.com>
Co-authored-by: mbio <qnwlqnwlxm@naver.com>
Co-authored-by: bstorozhuk <storozhuk.b.m@gmail.com>
Co-authored-by: Mahmoud Romeh <mahmoud.romeh@collibra.com>
Co-authored-by: KrnSaurabh <39181662+KrnSaurabh@users.noreply.github.com>
Co-authored-by: Robert Winkler <robwin@t-online.de>
Co-authored-by: Dan Maas <daniel.maas@target.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@injae-kim injae-kim injae-kim left review comments

@RobWin RobWin RobWin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@qnwlqnwlxm @RobWin @injae-kim