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

concurrent-api: Add ability to cleanup resources on timeout #3035

Closed

Conversation

bryce-anderson
Copy link
Contributor

Motivation:

The Single.timeout(..) family of operations are part of a class of operations will short-circuit the response based on the cancellation pathway, specifically they will return a TimeoutException down the error path. Because cancellation and the result are intrinsically racy there is currently the possibility that the result will be generated but we cannot return it to the subscriber having already given them an Exception in the error pathway. For values that are stateful this can result in a resource leak.

Modifications:

  • Add a timeout combinator that includes a resource cleanup hook.
  • Use that hook in the AbstractTimeoutHttpFilter where we need to drain the message body which can include live and stateful resources such as cleanup hooks and even the life cycle management of a HTTP session.

Result:

One less leak and at least the option for users to cleanup stateful resources on timeout races.

Motivation:

The Single.timeout(..) family of operations are part of a class of
operations will short-circuit the response based on the cancellation
pathway, specifically they will return a TimeoutException down the
error path. Because cancellation and the result are intrinsically
racy there is currently the possibility that the result will be
generated but we cannot return it to the subscriber having already
given them an Exception in the error pathway. For values that are
stateful this can result in a resource leak.

Modifications:

- Add a timeout combinator that includes a resource cleanup hook.
- Use that hook in the AbstractTimeoutHttpFilter where we need to
  drain the message body which can include live and stateful
  resources such as cleanup hooks and even the life cycle management
  of a HTTP session.

Result:

One less leak and at least the option for users to cleanup stateful
resources on timeout races.
@bryce-anderson
Copy link
Contributor Author

Note that this is one of two leaks discovered in the CapacityLimiter lock-up investigation. More details can be found in #3033.

@bryce-anderson
Copy link
Contributor Author

Offline discussion: this will fix the problem but opens up the can of worms that many of our operators are not resource safe so it doesn't make sense to only address it here. We'll find another way.

@bryce-anderson bryce-anderson deleted the bl_anderson/timeout-leak branch August 9, 2024 17: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.

1 participant