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
Making sure client_base instance that registered the component does not unregister it when being destructed #2976
Conversation
76864c2
to
5741e55
Compare
Thanks, my problem solved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I'm not a big fan of the solution. It brings the shared state of futures, GIDs and symbolic names together in a non obvious way.
HPX_ASSERT(registered_name_.empty()); // call only once | ||
registered_name_ = name; | ||
if (manage_lifetime) | ||
hpx::agas::register_name(launch::sync, name, id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shared state should hold the id we want to register the name, right? Why do we need to pass in another one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that could be done, indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -296,6 +297,20 @@ namespace detail | |||
error_code& = throws) = 0; | |||
virtual std::exception_ptr get_exception_ptr() const = 0; | |||
|
|||
virtual std::string const& get_registered_name() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really required to be part of the abstract base? We only need it for the id_type specialization, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's only needed for that. The alternative is to upcast the shared state - which I'd like to avoid.
Any suggestion on how to solve this otherwise? |
5741e55
to
f38b014
Compare
…ot unregister it when being destructed
f38b014
to
204a1a5
Compare
I think the change to future_base was unnecessary. If I'm not completely mistaken, it would have been more then enough if client_base got it's own special shared state. No upcasting or dowancasting needed since the type is known in the context of name registration. Otherwise you'd need an extra check of some kind anyway to avoid the exception that the virtual function is not implemented. The merge was a little too hasty, IMHO. |
@sithhell No, a special (and independent) shared state wouldn't have been enough. We need for The current solution creates a 'special shared state' but it does so in a way that still keeps it compatible with the shared state of |
@sithhell <https://github.com/sithhell> No, a special (and independent)
shared state wouldn't have been enough. We need for client_base<> to be
directly constructible from a future<id_type> (in fact, client_base is
nothing but a special version of a future<id_type>). While this could be
done still if those had independent shared states, it would require
additional unwrapping etc. to make it happen (that's what I wanted to
avoid).
Yes, it would require special unwrapping. I would not consider that a deal
breaker though.
The current solution creates a 'special shared state' but it does so in a
way that still keeps it compatible with the shared state of future<id_type>.
True and creates a dependency between `id_type` and `future_data`. The code
as is is suspect to odr violations. Not all TUs that use `future<id_type>`
include the `client_base` header.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2976 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AADRl6buJLbnXtMeC0-q3ltpdWH5LiqZks5sxPDwgaJpZM4QHtwP>
.
|
I'd consider this to be a deal-breaker.
Those compilation units wouldn't compile. But overall, please feel free to come up with a different solution. |
Yes, it would require special unwrapping. I would not consider that a deal
breaker though.
I'd consider this to be a deal-breaker.
The code as it is suspect to odr violations. Not all TUs that use
future<id_type> include the client_base header.
Those compilation units wouldn't compile.
So we now need to include client_base.hpp every time we use
`future<id_type>`? That sounds intrusive, we'll see how it goes.
But overall, please feel free to come up with a different solution.
Well, I don't like the client design in general. So I'd go for a completely
different solution. But that's a different discussion.
I'm questioning the coupling this patch creates between otherwise kind of
independent parts. Your milage obviously varies. I don't have another
solution, I'm just pointing at potential problems of this solution.
|
@Hapoo please verify whether this fixes your problem