refactor(http/upgrade): Http11Upgrade is Clone
#3540
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cratelyn
commented
Jan 17, 2025
this is a noöp change, to set the stage for subsequent changes to the internal model of `Http11Upgrade`. this `inner` field will shortly be an option, and this will make it easier to only follow these panicking branches when the inner lock is `Some(_)`. Signed-off-by: katelyn martin <[email protected]>
this commit hinges on this change to the upgrade middleware's `inner`
field.
we still retain a reference-counted copy of the `Inner` state, but now
we may store `None` here.
```
pub struct Http11Upgrade {
half: Half,
- inner: Arc<Inner>,
+ inner: Option<Arc<Inner>>,
}
```
a new branch is added to the `insert_half` method that consumes the
"sender" and inserts an upgrade future; when this is `None` it will do
nothing, rather than panicking.
Signed-off-by: katelyn martin <[email protected]>
this type is an empty flag to indicate whether an `Http11Upgrade` extension corresponds to the server or client half of the upgrade future channel. this type is made copy, to facilitate making the `Http11Upgrade` extension safely cloneable. Signed-off-by: katelyn martin <[email protected]>
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]>
cratelyn
force-pushed
the
kate/http-1.1-upgrade-middleware-is-clone
branch
from
2231b8b to
476aa04
Compare
olix0r
reviewed
Jan 21, 2025
Signed-off-by: katelyn martin <[email protected]>
Signed-off-by: katelyn martin <[email protected]>
`FutureExt::map_ok()` won't work if we try to return an error from this block. the `and_then()` adaptor is used to chain futures, and also won't work given a synchronous closure. this can be done with the equivalent `.await` syntax, and leaves a nicer hole for us to propagate other errors here, shortly. Signed-off-by: katelyn martin <[email protected]>
#3540 (comment) Signed-off-by: katelyn martin <[email protected]> Co-Authored-By: Oliver Gould <[email protected]>
Signed-off-by: katelyn martin <[email protected]>
olix0r
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
see linkerd/linkerd2#8733.
in the 1.0 interface of the
httpcrate, request and response extensions mustnow satisfy a
Clonebound.Http11Upgradewas written before this was thecase, 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 wereto clone a request or response's extensions. it might call
insert_half()a second time, and discover that theTryLock<T>hadalready been set.
moreover, holding on to a copy of the extensions would prevent the
Dropmethod forInnerfrom being called. This would causeconnections that negotiate an HTTP/1.1 upgrade to deadlock due to the
OnUpgradefutures never being polled, and failing to create aDuplexthat acts as the connection's I/O transport.
this commit makes use of the alterations to
Http11Upgrademade inprevious commits, and adds a safe implementation of
Clone. byonly 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.