Skip to content
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

Jump to bottom

Fixing held locks #1697

Merged
merged 3 commits into from Jul 31, 2015
Merged

Fixing held locks #1697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a52823e
Fixing warnings on MSVC, replace __PRETTY_FUNCTION__, add solution fo…
hkaiser Jul 30, 2015
dae7d15
Fixing held locks while suspension
hkaiser Jul 31, 2015
8a726a0
Addressing review comments
hkaiser Jul 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 15 additions & 3 deletions hpx/lcos/detail/future_data.hpp
View file
Open in desktop
Expand Up @@ -300,6 +300,13 @@ namespace detail
wait(ec);
if (ec) return NULL;

// No locking is required. Once a future has been made ready, which
// is a postcondition of wait, either:
//
// - there is only one writer (future), or
// - there are multiple readers only (shared_future, lock hurts
// concurrency)

if (state_ == empty) {
// the value has already been moved out of this future
HPX_THROWS_IF(ec, no_state,
Expand All @@ -309,7 +316,7 @@ namespace detail
}

// the thread has been re-activated by one of the actions
// supported by this promise (see \a promise::set_event
// supported by this promise (see promise::set_event
// and promise::set_exception).
if (state_ == exception)
{
Expand Down Expand Up @@ -388,6 +395,7 @@ namespace detail

// check whether the data has already been set
if (is_ready_locked()) {
l.unlock();
Copy link
Member

Choose a reason for hiding this comment

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

This code can only be executed on a precondition violation, which would point to bugs elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, it's an unrelated change.

HPX_THROWS_IF(ec, promise_already_satisfied,
"future_data::set_value",
"data has already been set for this future");
Expand Down Expand Up @@ -423,6 +431,7 @@ namespace detail

// check whether the data has already been set
if (is_ready_locked()) {
l.unlock();
Copy link
Member

Choose a reason for hiding this comment

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

Does HPX_THROWS suspend? Is that the origin of the held lock exceptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

THROWS might suspend as the stack is collected on a different HPX thread under certain circumstances, this has nothing to do with the problem at hand, it's just a fly-by.

HPX_THROWS_IF(ec, promise_already_satisfied,
"future_data::set_exception",
"data has already been set for this future");
Expand All @@ -436,7 +445,8 @@ namespace detail
// set the data
boost::exception_ptr* exception_ptr =
static_cast<boost::exception_ptr*>(storage_.address());
::new ((void*)exception_ptr) boost::exception_ptr(std::forward<Target>(data));
::new ((void*)exception_ptr) boost::exception_ptr(
std::forward<Target>(data));
state_ = exception;

// handle all threads waiting for the future to become ready
Expand Down Expand Up @@ -489,7 +499,8 @@ namespace detail
/// operation. Allows any subsequent set_data operation to succeed.
void reset(error_code& /*ec*/ = throws)
{
boost::unique_lock<mutex_type> l(this->mtx_);
// no locking is required as semantics guarantee a single writer
// and no reader

// release any stored data and callback functions
switch (state_) {
Expand All @@ -509,6 +520,7 @@ namespace detail
}
default: break;
}

state_ = empty;
on_completed_ = completed_callback_type();
}
Expand Down
13 changes: 5 additions & 8 deletions hpx/lcos/future.hpp
View file
Open in desktop
Expand Up @@ -659,19 +659,16 @@ namespace hpx { namespace lcos
private:
struct invalidate
{
explicit invalidate(future& f, bool reset_data = false)
: f_(f), reset_data_(reset_data)
explicit invalidate(future& f)
: f_(f)
{}

~invalidate()
{
if (reset_data_)
f_.shared_state_->reset();
f_.shared_state_ = 0;
f_.shared_state_.reset();
}

future& f_;
bool reset_data_;
};

private:
Expand Down Expand Up @@ -800,7 +797,7 @@ namespace hpx { namespace lcos
"this future has no valid shared state");
}

invalidate on_exit(*this, true);
invalidate on_exit(*this);

typedef typename shared_state_type::result_type result_type;
result_type* result = this->shared_state_->get_result();
Expand All @@ -820,7 +817,7 @@ namespace hpx { namespace lcos
return detail::future_value<R>::get_default();
}

invalidate on_exit(*this, true);
invalidate on_exit(*this);

typedef typename shared_state_type::result_type result_type;
result_type* result = this->shared_state_->get_result(ec);
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/serialization/polymorphic/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ foreach(test ${tests})
${${test}_FLAGS}
EXCLUDE_FROM_ALL
HPX_PREFIX ${HPX_BUILD_PREFIX}
FOLDER "Tests/Unit/Serialization/")
FOLDER "Tests/Unit/Serialization/Polymorphic")

add_hpx_unit_test("serialization" ${test} ${${test}_PARAMETERS})

Expand Down
6 changes: 3 additions & 3 deletions tests/unit/serialization/polymorphic/polymorphic_semiintrusive_template.cpp
View file
Open in desktop
Expand Up @@ -4,9 +4,6 @@
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#include <iostream>
#include <vector>

#include <hpx/runtime/serialization/serialize.hpp>
#include <hpx/runtime/serialization/base_object.hpp>
#include <hpx/runtime/serialization/shared_ptr.hpp>
Expand All @@ -15,6 +12,9 @@
#include <boost/shared_ptr.hpp>
#include <boost/make_shared.hpp>

#include <iostream>
#include <vector>

template <class T>
struct A
{
Expand Down
11 changes: 6 additions & 5 deletions tests/unit/serialization/polymorphic/polymorphic_template.cpp
View file
Open in desktop
Expand Up @@ -3,16 +3,17 @@
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#include <iostream>
#include <vector>

#include <hpx/runtime/serialization/serialize.hpp>
#include <hpx/runtime/serialization/base_object.hpp>
#include <hpx/runtime/serialization/shared_ptr.hpp>
#include <hpx/util/lightweight_test.hpp>

#include <boost/shared_ptr.hpp>
#include <boost/make_shared.hpp>
#include <boost/current_function.hpp>

#include <iostream>
#include <vector>

template <class T>
struct A
Expand All @@ -31,7 +32,7 @@ struct A
void serialize(Ar& ar, unsigned)
{
ar & a;
std::cout << __PRETTY_FUNCTION__ << std::endl;
std::cout << BOOST_CURRENT_FUNCTION << std::endl;
}
HPX_SERIALIZATION_POLYMORPHIC_ABSTRACT(A);
};
Expand All @@ -53,7 +54,7 @@ struct B: A<T>
template <class Ar>
void serialize(Ar& ar, unsigned)
{
std::cout << __PRETTY_FUNCTION__ << std::endl;
std::cout << BOOST_CURRENT_FUNCTION << std::endl;
ar & hpx::serialization::base_object<A<T> >(*this);
ar & b;
}
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/serialization/polymorphic/smart_ptr_polymorphic.cpp
View file
Open in desktop
Expand Up @@ -3,8 +3,6 @@
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#include <iostream>

#include <hpx/runtime/serialization/serialize.hpp>
#include <hpx/runtime/serialization/base_object.hpp>
#include <hpx/runtime/serialization/shared_ptr.hpp>
Expand All @@ -15,6 +13,8 @@

#include <hpx/util/lightweight_test.hpp>

#include <iostream>

// =========================shared_ptr test==============================
struct A
{
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/serialization/polymorphic/smart_ptr_polymorphic_nonintrusive.cpp
View file
Open in desktop
Expand Up @@ -3,8 +3,6 @@
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#include <iostream>

#include <hpx/runtime/serialization/serialize.hpp>
#include <hpx/runtime/serialization/base_object.hpp>
#include <hpx/runtime/serialization/shared_ptr.hpp>
Expand All @@ -15,6 +13,8 @@

#include <hpx/util/lightweight_test.hpp>

#include <iostream>

// =========================shared_ptr test==============================
struct A
{
Expand Down