Skip to content

Commit

Permalink
refactor(http/upgrade): Http11Upgrade is Clone
Browse files Browse the repository at this point in the history
this commit makes `Http11Upgrade` a cloneable type.

see <linkerd/linkerd2#8733>.

in the 1.0 interface of the `http` crate, request and response
extensions must now satisfy a `Clone` bound.

`Http11Upgrade` was written before this was the case, and is very
intentionally designed around the idea that it *not* be cloneable.

`insert_half()` in particular could cause the proxy to panic if it were
to clone a request or response's extensions. it might call
`insert_half()` a second time, and discover that the `TryLock<T>` had
already been set.

moreover, holding on to a copy of the extensions would prevent the
`Drop` method for `Inner` from being called. This would cause
connections that negotiate an HTTP/1.1 upgrade to deadlock due to the
`OnUpgrade` futures never being polled, and failing to create a `Duplex`
that acts as the connection's I/O transport.

this commit makes use of the alterations to `Http11Upgrade` made in
previous commits, and adds a *safe* implementation of `Clone`. by
only shallowly copying the extension, we tie the upgrade glue to a
*specific* request/response.

the extension can be cloned, but any generated copies will be inert.

Signed-off-by: katelyn martin <[email protected]>
  • Loading branch information
cratelyn committed Jan 21, 2025
1 parent 1b8c102 commit 476aa04
Showing 1 changed file with 18 additions and 2 deletions.
20 changes: 18 additions & 2 deletions linkerd/http/upgrade/src/upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ use try_lock::TryLock;
/// inserted into the `Request::extensions()`. If the HTTP1 client service
/// also detects an upgrade, the two `OnUpgrade` futures will be joined
/// together with the glue in this type.
// Note: this relies on there only having been 2 Inner clones, so don't
// implement `Clone` for this type.
pub struct Http11Upgrade {
half: Half,
inner: Option<Arc<Inner>>,
Expand Down Expand Up @@ -126,6 +124,24 @@ impl fmt::Debug for Http11Upgrade {
}
}

/// An [`Http11Upgrade`] can be cloned.
///
/// NB: Only the original copy of this extension may insert an [`OnUpgrade`] future into its half
/// of the channel. Calling [`insert_half()`] on any clones of an upgrade extension will be a noöp.

Check warning on line 130 in linkerd/http/upgrade/src/upgrade.rs

View workflow job for this annotation

GitHub Actions / rust

warning: unresolved link to `insert_half` --> linkerd/http/upgrade/src/upgrade.rs:130:31 | 130 | /// of the channel. Calling [`insert_half()`] on any clones of an upgrade extension will be a noöp. | ^^^^^^^^^^^^^ no item named `insert_half` in scope | = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
/// See the [`Drop`] implementation provided by [`Inner`] for more information.

Check warning on line 131 in linkerd/http/upgrade/src/upgrade.rs

View workflow job for this annotation

GitHub Actions / rust

warning: public documentation for `<unknown>` links to private item `Inner` --> linkerd/http/upgrade/src/upgrade.rs:131:51 | 131 | /// See the [`Drop`] implementation provided by [`Inner`] for more information. | ^^^^^ this item is private | = note: this link will resolve properly if you pass `--document-private-items` = note: `#[warn(rustdoc::private_intra_doc_links)]` on by default
impl Clone for Http11Upgrade {
fn clone(&self) -> Self {
Self {
half: self.half,
// We do *NOT* deeply clone our reference to `Inner`.
//
// `Http11Upgrade::insert_half()` and the `Inner` type's `Drop` glue rely on there only
// being one copy of the client and sender halves of the upgrade channel.
inner: None,
}
}
}

/// When both halves have dropped, check if both sides are inserted,
/// and if so, spawn the upgrade task.
impl Drop for Inner {
Expand Down

0 comments on commit 476aa04

Please sign in to comment.