Check 'closed_' in ExchangeClient::next() #9658
Open
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
Summary: Using Zombie Tasks we detected that Drviers can end up referenced by the lambdas waiting on the promises to be fulfilled. Promises given by the Exchange. Now, when Exchange is being closed, than everything under it (ExchangeClients and ExchangeQueues) are beling closed too, fulfilling any outstanding promises. The issue is that ExchangeClient allows to new promises being created in the next() call after we are closed(). This creates a situation where these promises are never fulfilled, because there is a proteciton to not call the fulfilling any outstanding promises more than once. The toot cause here is that next() does not respect 'closed_' flag and simply proceeds with asking the underlying ExchnageQueue for data, which in turn creates the promise. The fix is to check the 'closed_' flag and return straight away. The fix fixed the Zombie Tasks in the E2E test I was using to reproduce the issue. GH issue for this: prestodb/presto#22550 Differential Revision: D56712493
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
Yuhta
approved these changes
May 1, 2024
xiaoxmeng
reviewed
May 2, 2024
| @@ -117,6 +117,11 @@ ExchangeClient::next(uint32_t maxBytes, bool* atEnd, ContinueFuture* future) { | |||
| std::vector<std::unique_ptr<SerializedPage>> pages; | |||
| { | |||
| std::lock_guard<std::mutex> l(queue_->mutex()); | |||
| if (closed_) { | |||
There was a problem hiding this comment.
It is good to have a check here but I think this can't totally prevent this from happening. We probably need closed_ flag inside the queue as queue close is not called with queue lock protection as exchange client close does.
There was a problem hiding this comment.
@xiaoxmeng
ExchangeQueue by itself it not protected, instead we rely on protection in the ExchangeClient.
I believe this problem is fully solved.
We will observe the zombie tasks and if anything occurs - we will notice and address it.
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.
Summary:
Using Zombie Tasks we detected that Drviers can end up referenced by the lambdas waiting on the promises to be fulfilled.
Promises given by the Exchange.
Now, when Exchange is being closed, than everything under it (ExchangeClients and ExchangeQueues) are beling closed too, fulfilling any outstanding promises.
The issue is that ExchangeClient allows to new promises being created in the next() call after we are closed().
This creates a situation where these promises are never fulfilled, because there is a proteciton to not call the fulfilling any outstanding promises more than once.
The root cause here is that next() does not respect 'closed_' flag and simply proceeds with asking the underlying ExchnageQueue for data, which in turn creates the promise.
The fix is to check the 'closed_' flag and return straight away.
The fix fixed the Zombie Tasks in the E2E test I was using to reproduce the issue.
GH issue for this: prestodb/presto#22550
Differential Revision: D56712493