westonpace commented on a change in pull request #8680:
URL: https://github.com/apache/arrow/pull/8680#discussion_r545167684
##########
File path: cpp/src/arrow/util/future_test.cc
##########
@@ -276,6 +282,593 @@ TEST(FutureSyncTest, Int) {
}
}
+TEST(FutureRefTest, ChainRemoved) {
+ // Creating a future chain should not prevent the futures from being deleted
if the
+ // entire chain is deleted
+ std::weak_ptr<FutureImpl> ref;
+ std::weak_ptr<FutureImpl> ref2;
+ {
+ auto fut = Future<>::Make();
+ auto fut2 = fut.Then(StatusOnly, [](const Status& status) { return
Status::OK(); });
+ ref = fut.impl_;
+ ref2 = fut2.impl_;
+ }
+ ASSERT_TRUE(ref.expired());
+ ASSERT_TRUE(ref2.expired());
+
+ {
+ auto fut = Future<>::Make();
+ auto fut2 = fut.Then(StatusOnly, [](const Status&) { return
Future<>::Make(); });
Review comment:
Future has released its locks by the time it calls into the callbacks so
it should be safe to access and won't cause deadlock. It's always going to be
redundant though since you should know the state of the future and have access
to the result from the callback arguments.
However, you could cause a memory leak. If a future is abandoned then the
callback will hold onto the future impl and the future impl holds onto the
callback.
I'm not sure what you can test here other than to verify it is a bad idea.
Do we need a comment warning users to not do this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
