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.

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

Memory management issue in hpx::lcos::promise #1826

Closed
hkaiser opened this issue Oct 28, 2015 · 7 comments

Comments

Projects
None yet
3 participants
@hkaiser
Copy link
Member

commented Oct 28, 2015

The following code causes issues if it is executed more than N*2^16 times (N varies on different platforms between 1, 2, or 3):

namespace detail
{
    hpx::future<hpx::id_type> on_register_event(hpx::future<bool> f,
        hpx::lcos::promise<hpx::id_type, hpx::naming::gid_type> p)
    {
        if (!f.get())
        {
            HPX_THROW_EXCEPTION(bad_request,
                "hpx::agas::detail::on_register_event",
                "request 'symbol_ns_on_event' failed");
            return hpx::future<hpx::id_type>();
        }
        return p.get_future();
    }
}

future<hpx::id_type> addressing_service::on_symbol_namespace_event(
    std::string const& name, namespace_action_code evt,
    bool call_for_past_events)
{
    if (evt != symbol_ns_bind)
    {
        HPX_THROW_EXCEPTION(bad_parameter,
            "addressing_service::on_symbol_namespace_event",
            "invalid event type");
        return hpx::future<hpx::id_type>();
    }

    lcos::promise<naming::id_type, naming::gid_type> p;
    request req(symbol_ns_on_event, name, evt, call_for_past_events, p.get_id());
    hpx::future<bool> f = stubs::symbol_namespace::service_async<bool>(
        name, req, action_priority_);

    using util::placeholders::_1;
    return f.then(util::bind(
            util::one_shot(&detail::on_register_event),
            _1, std::move(p)
        ));
}

The problem does not manifest itself if we move the future instead of moving the promise to the continuation (i.e. instead of std::move(p) we write std::move(p.get_future())).

@K-ballo

This comment has been minimized.

Copy link
Member

commented Oct 28, 2015

std::move(p.get_future()) sounds redundant, doesn't promise::get_future yield an rvalue already?

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2015

@K-ballo indeed.

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2015

I think this is related to the issue described by #1620: Revert #1535

@sithhell

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

Unfortunately, it doesn't fix #1620. That's a different issue. I am on to it!

@sithhell

This comment has been minimized.

Copy link
Member

commented Oct 29, 2015

See #1828 for a fix for #1620.

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2015

Unfortunately, it doesn't fix #1620. That's a different issue. I am on to it!

I don't think this would fix #1620. What I meant was that it's caused by the very same architectural decision of how promises manage their memory.

@hkaiser

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2015

This was fixed by merging #1827

@hkaiser hkaiser closed this Oct 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.