Adjust Worker::poll logic to fix pending wake failure in balance service.#825
Open
c98 wants to merge 2 commits intotower-rs:masterfrom
Open
Adjust Worker::poll logic to fix pending wake failure in balance service.#825c98 wants to merge 2 commits intotower-rs:masterfrom
c98 wants to merge 2 commits intotower-rs:masterfrom
Conversation
…ice. This commit aims to solve the problem that Worker::poll_next_msg may block service.poll_ready. Especially when the service is a balance service, while multiple pending endpoints become ready, the task is blocked on Worker::poll_next_msg if there is no other message come which would cause these endpoints to be disconnected.
Collaborator
|
Thanks! Looks like CI failed... Also, is there a way to update the tests to check this change? |
adjust the buffer tests. remove `propagates_trace_spans` test due to the worker's service `poll_ready` is outside the request processing.
Author
|
Thanks for your attention, I have updated the tests to check this change. |
Collaborator
|
@cratelyn do yall have a more extensive suite that includes balancing that can test this change out? |
Contributor
we do! i'll try running the linkerd2-proxy tests with this patch applied, pardon the wait. |
cratelyn
reviewed
May 21, 2025
Contributor
cratelyn
left a comment
cratelyn
left a comment
There was a problem hiding this comment.
i left some notes below. my main questions and concerns about this change are about how many of the test suite's assertions (and a test case) are removed in this change.
if changes to the test suite are required here, is this a breaking change to this middleware's behavior?
Comment on lines
-378
to
-395
| #[tokio::test(flavor = "current_thread")] | ||
| async fn propagates_trace_spans() { | ||
| use tower::util::ServiceExt; | ||
| use tracing::Instrument; | ||
|
|
||
| let _t = support::trace_init(); | ||
|
|
||
| let span = tracing::info_span!("my_span"); | ||
|
|
||
| let service = support::AssertSpanSvc::new(span.clone()); | ||
| let (service, worker) = Buffer::pair(service, 5); | ||
| let worker = tokio::spawn(worker); | ||
|
|
||
| let result = tokio::spawn(service.oneshot(()).instrument(span)); | ||
|
|
||
| result.await.expect("service panicked").expect("failed"); | ||
| worker.await.expect("worker panicked"); | ||
| } |
Contributor
There was a problem hiding this comment.
may i ask why this test was deleted?
Author
There was a problem hiding this comment.
this commit change the worker exec order:
- service.poll_ready
- poll_next_msg
- service.call
after 2 poll_next_msg, the span in the msg can be retrieved, the service.call is now in the span scope, but not the servce.poll_ready, as you see, AssertSpanSvc does not fit the case, so i delete this test.
Comment on lines
-348
to
-353
| let err = assert_ready_err!(response2.poll()); | ||
| assert!( | ||
| err.is::<error::ServiceError>(), | ||
| "response should fail with a ServiceError, got: {:?}", | ||
| err | ||
| ); |
Contributor
There was a problem hiding this comment.
why is this no longer the case?
Author
There was a problem hiding this comment.
as mentioned above, the old exec order is
- poll_next_msg
- service.poll_ready
- service.call
the new exec order is
- service.poll_ready
- poll_next_msg
- service.call
there is no 'preload' message, so i delete it.
Comment on lines
-220
to
-228
| assert_pending!(worker.poll()); | ||
| handle | ||
| .next_request() | ||
| .await | ||
| .unwrap() | ||
| .1 | ||
| .send_response("world"); | ||
| assert_pending!(worker.poll()); | ||
| assert_ready_ok!(response4.poll()); |
Contributor
There was a problem hiding this comment.
why are these assertions no longer needed?
Author
There was a problem hiding this comment.
because in this case there is no other request will be issued, so it is no need to do this assertion.
Contributor
|
if this change is focused on solving how this middleware interacts with a |
Contributor
@seanmonstar i wasn't able to get a patched version of the linkerd2-proxy building with this patch. we're on tonic v0.12 at the moment, which depends on tower 0.4, and thus builds fail if this branch is used. i did leave some questions above, however. i'm concerned that trace contexts no longer seem to be propagated, and that this change seems to change the behavior of |
Collaborator
|
@cratelyn you've since ported all of linkerd to use the newer hyper/tonic, I think, right? If it'd be easier to pop this branch and see if the test suite is still happy, that could help move this along. If you're busy, understandable! |
Contributor
i ran the linkerd-proxy test suite against this branch, via a tests did not pass, due to a new failure in this test: https://github.com/linkerd/linkerd2-proxy/blob/main/linkerd/stack/src/loadshed.rs#L133-L198 i have not dug further into the cause, but i hope this helps! |
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.
#824
This commit aims to solve the problem that
Worker::poll_next_msgmay blockservice.poll_ready. Especially when the service is aBalanceservice, while multiple pending endpoints become ready, the task is blocked onWorker::poll_next_msgif there is no other message come which would cause these endpoints to be disconnected.