Skip to content

CPP-987 Make handling of future callbacks play nicely with the predicate guards around cass_future_wait #559

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 28, 2023
Merged

CPP-987 Make handling of future callbacks play nicely with the predicate guards around cass_future_wait #559

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a767a16
Play nicely with the predicate guards in the future wait operation
Sep 25, 2023
4b533c9
Clean up comment a bit
Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/future.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,14 +189,18 @@ bool Future::set_callback(Future::Callback callback, void* data) {
}

void Future::internal_set(ScopedMutex& lock) {
is_set_ = true;
if (callback_) {
Callback callback = callback_;
void* data = data_;
lock.unlock();
callback(CassFuture::to(this), data);
lock.lock();
}

// CPP-987 Set this after the callbacks run to avoid unexpected exits
// from wait ops due to spurious wakeups
is_set_ = true;

Copy link
Contributor Author

@absurdfarce absurdfarce Sep 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this will get mentioned in the release notes for the corresponding release as a possible change in behaviour for callbacks. In reality it shouldn't result in much of a change at all; it might change behaviours for very aberrant cases (like, say, calling cass_future_wait() in the callback) but any such behaviours are likely undocumented (and almost certainly wrong).

Copy link

@blake-edwards-n blake-edwards-n Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this also prevent calling cass_future_get_result in the callback? Is there a problem with that otherwise? Or another recommended way to handle processing the results without blocking the call that invoked the query?

Copy link

@jayumgohil jayumgohil Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, We are facing the same issue, as in our case the async processing expects that cb_fn() i.e. callback function should be able to call cass_future_error_code() or cass_future_get_result() and process the results for further action.

Rather, please try to run build/examples/callbacks with this change,
We can see a hang there itself:

(gdb) bt
#0  futex_wait_cancelable (private=0, expected=0, futex_word=0x61ee58) at ../sysdeps/unix/sysv/linux/futex-internal.h:88
#1  __pthread_cond_wait_common (abstime=0x0, mutex=0x61ee00, cond=0x61ee30) at pthread_cond_wait.c:502
#2  __pthread_cond_wait (cond=0x61ee30, mutex=0x61ee00) at pthread_cond_wait.c:655
#3  0x00007ffff718421d in uv_cond_wait (cond=<optimized out>, mutex=<optimized out>) at src/unix/thread.c:780
#4  0x00007ffff782bf6e in datastax::internal::core::Future::internal_wait(datastax::internal::ScopedLock<datastax::internal::Mutex>&) () from /root//cpp-driver/build/libcassandra.so.2
#5  0x00007ffff782beea in datastax::internal::core::Future::error() () from /root//cpp-driver/build/libcassandra.so.2
#6  0x00007ffff782b87c in cass_future_error_code () from /root//cpp-driver/build/libcassandra.so.2
#7  0x0000000000401973 in on_session_connect ()
#8  0x00007ffff782bd9f in datastax::internal::core::Future::internal_set(datastax::internal::ScopedLock<datastax::internal::Mutex>&) () from /root//cpp-driver/build/libcassandra.so.2
#9  0x00007ffff7898bbc in datastax::internal::core::Future::set() () from /root//cpp-driver/build/libcassandra.so.2
#10 0x00007ffff7897ecd in datastax::internal::core::SessionBase::notify_connected() () from /root//cpp-driver/build/libcassandra.so.2
#11 0x00007ffff7894c6c in datastax::internal::core::SessionInitializer::on_initialize(datastax::internal::core::RequestProcessorInitializer*) () from /root//cpp-driver/build/libcassandra.so.2
#12 0x00007ffff7896fe5 in datastax::internal::Callback<void, datastax::internal::core::RequestProcessorInitializer*>::MemberInvoker<void (datastax::internal::core::SessionInitializer::*)(datastax::internal::core::RequestProcessorInitializer*), datastax::internal::core::SessionInitializer>::invoke(datastax::internal::core::RequestProcessorInitializer* const&) const () from /root//cpp-driver/build/libcassandra.so.2
#13 0x00007ffff788551e in datastax::internal::Callback<void, datastax::internal::core::RequestProcessorInitializer*>::operator()(datastax::internal::core::RequestProcessorInitializer* const&) const ()
   from /root//cpp-driver/build/libcassandra.so.2
#14 0x00007ffff7884c7a in datastax::internal::core::RequestProcessorInitializer::on_initialize(datastax::internal::core::ConnectionPoolManagerInitializer*) () from /root//cpp-driver/build/libcassandra.so.2

Copy link

@jayumgohil jayumgohil Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@absurdfarce @hhughes @blake-edwards-n @joao-r-reis

Copy link
Contributor Author

@absurdfarce absurdfarce Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on a quick review it very much looks like the analysis above is correct. I strongly suspect we'll have to revert the prior fix and figure out another way to handle CPP-987.

Copy link
Contributor Author

@absurdfarce absurdfarce Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To follow up here: the changes in CPP-987 have been reverted. I'm going to start working on finding another way to resolve those issues.

// Broadcast after we've run the callback so that threads waiting
// on this future see the side effects of the callback.
uv_cond_broadcast(&cond_);
Expand Down