-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make sure we rethrow exceptions in async tasks #355
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zhengbuqian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
b21a90c
to
3ae4e82
Compare
No. Basically, we want to wait for all the futures to finish and only then to process the exception. Alternatively, we could have something that stops further futures from getting spawned if one of them experienced a problem, but this is not really needed. I've tried the following snippet:
And this code may produce the following:
or
This is wrong, because it means that some futures continue working after the synchronization code What we'd like ideally to have is
The original snippet
produces
which is a desired behavior. |
/hold |
3ae4e82
to
94970b6
Compare
4cabac9
to
89f8e5e
Compare
…olly::Future::wait but not trying to get the values; use folly::collect to simplify code Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
89f8e5e
to
cbaf51b
Compare
thanks @alexanderguzhva for the clarification! I should have used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collectAll
works, but possible memory leaks need to be addressed. Please change T* x = new T[N];
into std::vector<T> x(N);
or std::unique_ptr<T[]> x(N);
whenever possible.
} | ||
|
||
bool failed = TryDiskANNCall([&]() { WaitAllSuccess(futures); }) != Status::success; | ||
|
||
if (warmup != nullptr) { | ||
diskann::aligned_free(warmup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a memory leak in case of exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TryDiskANNCall catches all exceptions and returns non success Status, thus memory leak won't happen here.
for (auto &future : futures) { | ||
future.wait(); | ||
} | ||
knowhere::WaitAllSuccess(futures); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memory leaks in case of exception
if (samples != nullptr)
delete[] samples;
if (pq_code != nullptr)
delete[] pq_code;
for (auto &future : futures) { | ||
future.wait(); | ||
} | ||
knowhere::WaitAllSuccess(futures); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a memory leak in case of exception
delete[] stats;
for (auto &future : futures) { | ||
future.wait(); | ||
} | ||
knowhere::WaitAllSuccess(futures); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memory leaks in case of exception
delete[] distances;
delete[] center;
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Alex!
} | ||
|
||
bool failed = TryDiskANNCall([&]() { WaitAllSuccess(futures); }) != Status::success; | ||
|
||
if (warmup != nullptr) { | ||
diskann::aligned_free(warmup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TryDiskANNCall catches all exceptions and returns non success Status, thus memory leak won't happen here.
/lgtm |
/unhold |
in many places where folly::Future is used, we only wait but not try to get the value. if the async task thrown an exception wait() will unblock but not rethrow it, causing the async thread to crash.
now using folly::collect().get() to make sure we always wait for all futures and rethrow exceptions so callers may catch and handle them.
/kind improvement