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

Fix a bug where the parent's timeout is not cancelled in RetryingClient #5896

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Sep 9, 2024

Motivation:

Originally, when a request is derived, the parent's timeout scheduler is not canceled. This was not a problem until the ResponseTimeoutMode.FROM_START feature was added because the timeout did not start at this point.

When ResponseTimeoutMode.FROM_START is set, a timeout task is scheduled before it is derived. So it needs to be canceled. Otherwise, the uncanceled scheduler task may cause a leak because referenced objects are not GC'd until the task is completed.

Modifications:

  • Cancel the timeout scheduler of the parent request when it is derived.

Result:

Fixed a bug where timeout tasks leak when using ResponseTimeoutMode.FROM_START with RetryingClient.

…ent`

Motivation:

Originally, when a request is derived, the parent's timeout scheduler is
not canceled. This was not a problem until the `ResponseTimeoutMode.FROM_START`
feature was added, because the timeout did not start at this point. If `ResponseTimeoutMode.FROM_START`
is set, a timeout task is scheduled before it is derived so the uncanceled
scheduler task may cause a leak because referenced objects are not GC'd
until the task is completed.

Modifications:

- Cancel the timeout scheduler of the parent request when it is derived.

Result:

Fixed a bug where timeout tasks leak when using `ResponseTimeoutMode.FROM_START`
with `RetryingClient`.
@ikhoon ikhoon added the defect label Sep 9, 2024
@ikhoon ikhoon added this to the 1.30.1 milestone Sep 9, 2024
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@ikhoon ikhoon merged commit e0192cc into line:main Sep 10, 2024
14 of 15 checks passed
@ikhoon ikhoon deleted the retry-timeout-cancel branch September 10, 2024 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants