Simplify promise #2223
Merged
Simplify promise #2223
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
Calling package_action::get_future after package_action::apply might lead to a rare race condition in the lifetime management of the lcos::promise shared state
- flyby: adding (now) missing header
Making sure that the managed id return from the promise actually contains credit that keeps the shared state alive.
- make sure that traits usage has proper headers
- flyby: adding missing #includes
Calling package_action::get_future after package_action::apply might lead to a rare race condition in the lifetime management of the lcos::promise shared state
- flyby: adding (now) missing header
This patch is simplifying hpx::lcos::promise by defining it in terms of hpx::lcos::local::detail::promise_base and a separate class which implements the lco interface. This decouples the future shared state reference counting from the reference counting needed for the LCO component. We now have local only reference counting.
Conflicts: hpx/lcos/detail/async_implementations.hpp hpx/lcos/promise.hpp tests/unit/agas/components/managed_refcnt_checker.hpp tests/unit/agas/components/simple_refcnt_checker.hpp tests/unit/parcelset/put_parcels.cpp
This was referenced Jun 22, 2016
| @@ -0,0 +1,222 @@ | |||
| // Copyright (c) 2007-2015 Hartmut Kaiser | |||
There was a problem hiding this comment.
All of this file: please don't align assignments, variable names, etc.
|
General question: will the shared state be ever released if the value is never set? I.e. will this be memory-leak free: ? |
| "this promise has no valid LCO"); | ||
| return naming::invalid_id; | ||
| } | ||
| if (!this->future_retrieved_) |
There was a problem hiding this comment.
Do you still have to ensure that the future is retrieved before the get_id() was called? Or is that a left-over?
There was a problem hiding this comment.
We still have to do that.
| @@ -308,21 +310,20 @@ namespace hpx { namespace lcos | |||
| << hpx::actions::detail::get_action_name<action_type>() | |||
| << ", " << id << ") args(" << sizeof...(Ts) << ")"; | |||
|
|
|||
| auto cb = util::bind(&packaged_action::parcel_write_handler, | |||
| this->shared_state_, _1, _2); | |||
There was a problem hiding this comment.
Same here and more places below.
|
All in all, LGTM. Please correct the minor issues and we can go ahead with this. Thanks a lot! |
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.
This patch introduces a simplified redesign for
hpx:lcos::promise.It tries to simplify the current implementation by splitting up the shared_state, promise and lco component into three different classes. This makes the referencing scheme a little more obvious and therefor is able to remove the global reference counting alltogether by creating a cycle between the component and the shared state which is explicitly broken once the shared_state is set ready.