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

close jetty connections on monix timeout (or other cancellation) #1303

Merged
merged 8 commits into from
Dec 23, 2024

Conversation

halotukozak
Copy link
Member

@halotukozak halotukozak commented Dec 11, 2024

A config for jetty client can have idleTimeout property set to 2h (that means each client connection is closed after 2h if no communication is sent over the connection). This behavior may don't match e.g. 60 seconds monix task timeout setting. Long running http requests inside monix task causes the task to cancel without closing underlying jetty client connection session. Jetty client connection pool is then used up and accepts no more requests effectively blocking the communication.

@ddworak ddworak self-requested a review December 12, 2024 09:13
@halotukozak halotukozak force-pushed the close-stale-jetty-connections-on-monix-timeout branch from 5830008 to a161834 Compare December 17, 2024 12:50
@halotukozak halotukozak requested a review from ddworak December 17, 2024 14:17
@halotukozak halotukozak force-pushed the close-stale-jetty-connections-on-monix-timeout branch from cb56a4d to b00c43b Compare December 18, 2024 15:01
@halotukozak halotukozak requested a review from ddworak December 23, 2024 09:20
Updated test syntax to use `in` for better readability and migrated to fully asynchronous assertions where applicable. Removed redundant `.futureValue` statements, ensuring non-blocking behavior across test methods.
@halotukozak halotukozak force-pushed the close-stale-jetty-connections-on-monix-timeout branch from 9a35f65 to 01cf9f0 Compare December 23, 2024 14:13
Copy link
Member

@ddworak ddworak left a comment

Choose a reason for hiding this comment

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

LGTM with nits

Clarify the purpose of HttpClient configurations in `SttpRestCallTest` and `JettyRestCallTest`. The added comments explain the use of a connection timeout significantly exceeding the CallTimeout value, improving code readability and maintainability.
@ddworak ddworak merged commit e991b56 into master Dec 23, 2024
4 checks passed
@ddworak ddworak deleted the close-stale-jetty-connections-on-monix-timeout branch December 23, 2024 14:52
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