Skip to content

spawn-ready: Abort background tasks when the service is dropped#581

Merged
olix0r merged 6 commits intotower-rs:masterfrom
olix0r:ver/spawn-ready-drop
Apr 27, 2021
Merged

spawn-ready: Abort background tasks when the service is dropped#581
olix0r merged 6 commits intotower-rs:masterfrom
olix0r:ver/spawn-ready-drop

Conversation

@olix0r
Copy link
Collaborator

@olix0r olix0r commented Apr 26, 2021

The SpawnReady service spawns a background task whenever the inner
service is not ready; but when the SpawnReady service is dropped, this
task continues to run until it becomes ready (at which point the ready
service will be dropped). This can cause resource leaks when the inner
service never becomes ready.

This change adds a Drop implementation for the SpawnReady service
that aborts the background task when appropriate.

The `SpawnReady` service spawns a background task whenever the inner
service is not ready; but when the `SpawnReady` service is dropped, this
task continues to run until it becomes ready (at which point the ready
service will be dropped). This can cause resource leaks when the inner
service never becomes ready.

This change adds a `Drop` implementation for the `SpawnReady` service
that aborts the background task when appropriate.
@olix0r olix0r requested a review from hawkw April 26, 2021 23:58
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lgtm, modulo one note about where the test ought to go.

hawkw and others added 3 commits April 27, 2021 10:05
move abort_on_drop test to tests/spawn_ready
Signed-off-by: Eliza Weisman <[email protected]>
@olix0r olix0r merged commit 8ceee45 into tower-rs:master Apr 27, 2021
@olix0r olix0r deleted the ver/spawn-ready-drop branch April 27, 2021 17:26
hawkw added a commit that referenced this pull request Apr 27, 2021
# 0.4.7 (April 27, 2021)

### Added

- **builder**: Add `ServiceBuilder::check_service` to check the request,
    response, and error types of the output service. ([#576])
- **builder**: Add `ServiceBuilder::check_service_clone` to check the
  output service can be cloned. ([#576])

### Fixed

- **spawn_ready**: Abort spawned background tasks when the `SpawnReady`
  service is dropped, fixing a potential task/resource leak (#[581])
- Fixed broken documentation links ([#578])

[#576]: #576
[#578]: #578
[#581]: #581
hawkw added a commit that referenced this pull request Apr 27, 2021
# 0.4.7 (April 27, 2021)

### Added

- **builder**: Add `ServiceBuilder::check_service` to check the request,
    response, and error types of the output service. ([#576])
- **builder**: Add `ServiceBuilder::check_service_clone` to check the
  output service can be cloned. ([#576])

### Fixed

- **spawn_ready**: Abort spawned background tasks when the `SpawnReady`
  service is dropped, fixing a potential task/resource leak (#[581])
- Fixed broken documentation links ([#578])

[#576]: #576
[#578]: #578
[#581]: #581
hawkw added a commit to linkerd/linkerd2-proxy that referenced this pull request Apr 27, 2021
This updates `tower` to v0.4.7. This picks up tower-rs/tower#581, which
fixes a task leak in `SpawnReady` that resulted in a memory leak in the
proxy.
olix0r pushed a commit to linkerd/linkerd2-proxy that referenced this pull request Apr 28, 2021
This updates `tower` to v0.4.7. This picks up tower-rs/tower#581, which
fixes a task leak in `SpawnReady` that resulted in a memory leak in the
proxy.
olix0r pushed a commit to linkerd/linkerd2-proxy that referenced this pull request Apr 29, 2021
This updates `tower` to v0.4.7. This picks up tower-rs/tower#581, which
fixes a task leak in `SpawnReady` that resulted in a memory leak in the
proxy.
olix0r pushed a commit to linkerd/drain-rs that referenced this pull request Jun 3, 2021
This updates `tower` to v0.4.7. This picks up tower-rs/tower#581, which
fixes a task leak in `SpawnReady` that resulted in a memory leak in the
proxy.
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.

3 participants