Conversation
zaharidichev
force-pushed
the
zd/accept-future
branch
3 times, most recently
from
a6f3a53 to
1816bf9
Compare
zaharidichev
changed the title
olix0r
requested changes
Mar 31, 2020
hawkw
suggested changes
Mar 31, 2020
Contributor
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
I don't think most of the indirection in this PR is necessary. In general, we should only need to return Box<dyn Future> when we might conditionally return different concrete types that need to be erased by boxing. Otherwise, we should continue returning the same concrete types we did before.
zaharidichev
force-pushed
the
zd/accept-future
branch
from
58da728 to
a8a7d53
Compare
Signed-off-by: Zahari Dichev <[email protected]>
Signed-off-by: Zahari Dichev <[email protected]>
Signed-off-by: Zahari Dichev <[email protected]>
zaharidichev
force-pushed
the
zd/accept-future
branch
from
a8a7d53 to
6a0a622
Compare
hawkw
reviewed
Apr 1, 2020
Signed-off-by: Zahari Dichev <[email protected]>
olix0r
reviewed
Apr 8, 2020
Member
olix0r
left a comment
olix0r
left a comment
There was a problem hiding this comment.
This is looking pretty close. I need to check it out and browse through it myself but I left some superficial comments (tioli)
Signed-off-by: Zahari Dichev <[email protected]>
hawkw
approved these changes
Apr 9, 2020
Contributor
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
Overall, this change looks pretty good to me!
It would be good if there was an integration test asserting that a connection where no data is sent by the client times out at protocol detection correctly. I'm not sure how practical this is to test, but it would be nice to have.
In addition, I left some minor style comments. Hope they're helpful!
Member
Author
|
@hawkw I will add the test in a follow up PR if thats fine. At least it will be a good educational experience. |
olix0r
reviewed
Apr 10, 2020
olix0r
approved these changes
Apr 15, 2020
Member
olix0r
reviewed
Apr 15, 2020
linkerd/proxy/detect/src/lib.rs
Outdated
| target.take().expect("polled after complete"), | ||
| io.prefix().as_ref(), | ||
| ); | ||
| let inner_fut = accept.accept((target, BoxedIo::new(io))); |
Member
There was a problem hiding this comment.
Just noting that there was a bug lurking in this change: we were no longer polling the accept to ready before dispatching. These changes were just superficial anyway, it seems, so I've backed this out to be as it was before.
hawkw
approved these changes
Apr 15, 2020
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Apr 15, 2020
This release includes a new protocol detection timeout, which prevents clients from consuming resources indefinitely when they do not send any data. Additionally: the proxy's admin endpoint now supports a `/live` endpoint for liveness checks, and a feature has been added to enrich tracing metadata from a file of label/values. --- * Add Labels from a path as oc-collector attributes (linkerd/linkerd2-proxy#463) * Add liveness endpoint to admin server (linkerd/linkerd2-proxy#470) * docker: Use buildkit for caching (linkerd/linkerd2-proxy#472) * Makefile: Use STRIP variable with strip as default (linkerd/linkerd2-proxy#475) * Add checksec to the release process (linkerd/linkerd2-proxy#476) * Time out protocol detect futures (linkerd/linkerd2-proxy#464) * Ensure that checksec is executable (linkerd/linkerd2-proxy#477) * Fix the checksec URL (linkerd/linkerd2-proxy#478) * Undo hardcoded release version (linkerd/linkerd2-proxy#479)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Apr 16, 2020
This release includes a new protocol detection timeout, which prevents clients from consuming resources indefinitely when they do not send any data. Additionally: the proxy's admin endpoint now supports a `/live` endpoint for liveness checks, and a feature has been added to enrich tracing metadata from a file of label/values. --- * Add Labels from a path as oc-collector attributes (linkerd/linkerd2-proxy#463) * Add liveness endpoint to admin server (linkerd/linkerd2-proxy#470) * docker: Use buildkit for caching (linkerd/linkerd2-proxy#472) * Makefile: Use STRIP variable with strip as default (linkerd/linkerd2-proxy#475) * Add checksec to the release process (linkerd/linkerd2-proxy#476) * Time out protocol detect futures (linkerd/linkerd2-proxy#464) * Ensure that checksec is executable (linkerd/linkerd2-proxy#477) * Fix the checksec URL (linkerd/linkerd2-proxy#478) * Undo hardcoded release version (linkerd/linkerd2-proxy#479)
hawkw
added a commit
that referenced
this pull request
May 5, 2020
This branch updates `master-tokio-0.2` from the latest `master`. A lot of this diff is totally unrelated and is largely ignoreable (e.g. changes to the docker build configuration, etc). However, there was also a decently large chunk of code that had changed in both `master-tokio-0.2` and on master. The majority of the merge conflicts were related to the changes to `Accept` in #464, because `Accept` had also been updated to use `std::future` on `master-tokio-0.2`. A review should probably focus on the `Accept` codepaths. Signed-off-by: Eliza Weisman <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR changes the Accept trait to return a Future<Item = Future<()>>. The inner future is the one driving the connection while the outer future represents work such as protocol detect. This allows us to time out the outer future independently of the inner one. Additionally a timeout has been introduced to guard against serve first connections that do not run on a standard for the protocol port hanging and causing memory issues.
Fixes linkerd/linkerd2#4069
Signed-off-by: Zahari Dichev [email protected]