try fixing deadlock in xds when more than 65 services on the same port. - #2777
try fixing deadlock in xds when more than 65 services on the same port.#2777aryehlev wants to merge 3 commits into
Conversation
|
@aryehlev , thanks for the PR! It's indeed a crucial bug and exactly the kind of report we appreciate seeing from real production usage. The solution makes sense to me, especially that it matches the dispatching design of other grpc implementations mostly. I added two comments above. |
| struct ManyClustersServer; | ||
|
|
||
| #[tonic::async_trait] | ||
| impl AggregatedDiscoveryService for ManyClustersServer { |
There was a problem hiding this comment.
would be better to reuse the existing xds-test-util::XdsTestControlPlaneServic utility we created in-repo recently.
|
|
||
| /// Build the in-flight delivery future for one response's staged watcher | ||
| /// notifications, or `None` when there is nothing to deliver. | ||
| fn dispatch_pending(deliveries: Vec<Delivery>) -> Option<PendingDispatch> { |
There was a problem hiding this comment.
optimization suggestion: instead of sending Vec<Delivery> around, use refcount around one token, i.e. Arc<Option<Oneshot>> in ProcessingDone. I found out all the other grpc implementations use refcounts to track the updates dispatch.
There was a problem hiding this comment.
im not sure about this one, you mean to avoid this vec in line 833?
let mut done_rxs = Vec::with_capacity(deliveries.len());
There was a problem hiding this comment.
Sorry for the confusion, the comment is specifically about the oneshot channel in ProcessingDone. dispatch_resources need to do N(resources) * W(watchers) oneshot chanel allocations for each response. This is a pre-existing issue with the ProcessingDone design, not introduced by this PR. I recommend refactor ProcessingDone to use one Arc to track a single response now to both eliminate the allocations as well as simplifying Delivery shape here, it can drop the Receiver field.
2 participants
we have in our environment alot of versions of services all on the same port, this causes a deadlock in our xDS client at startup.
The shared route config for the port references more clusters than the client's internal queue can hold (64), and the two tasks that need each other to drain it end up waiting on each other.