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.

Sign up for GitHub

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

Jump to bottom

portforward: Improve API and support background task cancelation #854

Merged
merged 2 commits into from
Mar 22, 2022
Merged

portforward: Improve API and support background task cancelation #854

merged 2 commits into from
Mar 22, 2022

Conversation

olix0r
Copy link
Contributor

@olix0r olix0r commented Mar 22, 2022 •
edited
Loading

The portforward utility spawns a background task and returns a future
that completes when the background task completes, but the spawned
task's JoinHandle is discarded, so there's no way to cancel the
background tasks before all of the port forwards complete.

This change updates the Portforwarder type to hold the JoinHandle
and exposes a Portforward::abort method that cancels the spawned task.
impl Future for Portforward changes to await the background task's
completion. A portforward::Error variant is added to expose task
failures.

@olix0r
Copy link
Contributor Author

olix0r commented Mar 22, 2022

I'm realizing there's another issue with the port forward API: as far as I can tell, there's no way to take ownership of any of the Port streams (to, for instance, hand off to a hyper client, for instance). I'm going to propose opening up the API further to explicitly return port streams and a task so that the caller can determine what to do with them.

@kazk
Copy link
Member

kazk commented Mar 22, 2022

Thanks for this! The portforward PR (#446) was sitting there for about a year, and we basically merged to get further feedback like this from users. You are welcome to propose changes however you like 😆

@kazk kazk added the changelog-add changelog added category for prs label Mar 22, 2022
kazk
kazk approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@kazk kazk left a comment

Choose a reason for hiding this comment

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

LGTM. Just minor nit about #[from].

@olix0r
portforward: Propose API changes to support cancelation
bbb20da 
The `portforward` utility spawns a background task and returns a future
that completes when the background task completes, but the spawned
task's `JoinHandle` is discarded, so there's no way to cancel the
background tasks before all of the port forwards complete.

Furthermore, the `Portforwarder` types holds all `Port` instances and
exposes no APIs to allow a caller to take ownership. This makes it
difficult to work with a port stream. For example, it can't be moved
onto a spawned task directly. Instead a user has to extract the streams
(and errors) from the data structure.

To try to simplify this API:

* `Portforwarder` is updated to hold its streams and error handles in
  HashMaps keyed by port number. It is updated to expose `take_stream`
  and `take_error` methods so that callers can access either the I/O
  stream or the error handle by-numer. This is proposed to improve
  redability to avoid the unnecessary intermediate `Port` type.
* `Portforwarder` is updated to hold a `tokio::task::JoinHandle` and it
  now exposes `abort` and `join` methods that interact with the
  background task.
* `Portforward` no longer implements `Future` directly. This is done to
  reduce boilerplate code for what is now a very simple `async fn`.

Signed-off-by: Oliver Gould <ver@buoyant.io>
@olix0r olix0r force-pushed the ver/port-forward-cancelation branch from ad46471 to bbb20da Compare March 22, 2022 02:16
@olix0r
Fix spawn error type
c1806aa 
Signed-off-by: Oliver Gould <ver@buoyant.io>
@olix0r olix0r force-pushed the ver/port-forward-cancelation branch from 80d0efa to c1806aa Compare March 22, 2022 02:20
@olix0r
Copy link
Contributor Author

olix0r commented Mar 22, 2022 •
edited
Loading

@kazk Thanks for the quick review. I decided to propose a slightly larger API change after using the existing one. Let me know what you think. I'm happy to revert those changes, but I thought a shallower API was easier to follow. Let me know what you think!

@kazk kazk added changelog-change changelog change category for prs and removed changelog-add changelog added category for prs labels Mar 22, 2022
@kazk kazk changed the title portforward: Support background task cancelation portforward: Improve API and support background task cancelation Mar 22, 2022
kazk
kazk approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@kazk kazk left a comment

Choose a reason for hiding this comment

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

This is much better :)

clux
clux approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

wow, this is great. manages to both simplify the internals and the interface. very nice!

@clux clux added this to the 0.71.0 milestone Mar 22, 2022
@kazk kazk merged commit 116a970 into kube-rs:master Mar 22, 2022
@kazk
Copy link
Member

kazk commented Mar 22, 2022

I'll rewrite AttachedProcess like this, and remove all the panics.

@kazk kazk mentioned this pull request Mar 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clux clux clux approved these changes

@kazk kazk kazk approved these changes

Assignees
No one assigned
Labels
changelog-change changelog change category for prs
Projects
None yet
Milestone
0.71.0
Development

Successfully merging this pull request may close these issues.
3 participants
@olix0r @kazk @clux