Skip to content

Add more tests for TimedHandler#6343

Merged
shakuzen merged 4 commits intomicrometer-metrics:1.13.xfrom
kwondh5217:main
Jun 5, 2025
Merged

Add more tests for TimedHandler#6343
shakuzen merged 4 commits intomicrometer-metrics:1.13.xfrom
kwondh5217:main

Conversation

@kwondh5217
Copy link
Copy Markdown
Contributor

@kwondh5217 kwondh5217 commented Jun 3, 2025

This pull request addresses gh-6227.

This test verifies that TimedHandler.shutdown() does not complete while any in-flight requests are still processing.

Specifically, it covers the following scenario:

  • Two requests (/a and /b) are initiated concurrently

  • TimedHandler.shutdown() is called while both are still in progress

  • One request (/b) completes

  • shutdownFuture should remain incomplete until the other request (/a) also completes

This test verifies the shutdown behavior using only the internal request/response flow, without externally calling shutdown.check(), in line with the discussion in #6194 (comment)

@kwondh5217 kwondh5217 force-pushed the main branch 4 times, most recently from 75e9a95 to 9533d63 Compare June 3, 2025 09:29
Copy link
Copy Markdown
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thank you for working on this. I left a review.

Copy link
Copy Markdown
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thanks for the quick update. I've left another review.

@kwondh5217 kwondh5217 requested a review from shakuzen June 3, 2025 11:33
@kwondh5217
Copy link
Copy Markdown
Contributor Author

@shakuzen Thanks for the review!

I updated the test to wait for the onComplete() callback by overriding it in a custom TestableTimedHandler, as you suggested.

Please take another look when you have time 🙇

@kwondh5217 kwondh5217 force-pushed the main branch 2 times, most recently from 3be388c to 5e36e1a Compare June 4, 2025 12:38
Copy link
Copy Markdown
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the pull request and all the quick updates in response to feedback. I'll rebase it on 1.13.x and merge forward from there.

…ts finish

Closes micrometer-metricsgh-6227

Signed-off-by: Daeho Kwon <trewq231@naver.com>
Closes micrometer-metricsgh-6227

Signed-off-by: Daeho Kwon <trewq231@naver.com>
Closes micrometer-metricsgh-6227

Signed-off-by: Daeho Kwon <trewq231@naver.com>
Signed-off-by: Daeho Kwon <trewq231@naver.com>
@shakuzen shakuzen changed the base branch from main to 1.13.x June 5, 2025 02:09
@shakuzen shakuzen merged commit 61c904d into micrometer-metrics:1.13.x Jun 5, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants