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

A strange thing in parallel::util::detail::handle_local_exceptions. #2818

Closed
taeguk opened this Issue Aug 11, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@taeguk
Member

taeguk commented Aug 11, 2017

HPX_ATTRIBUTE_NORETURN static void call(std::exception_ptr const& e)
{
try {
std::rethrow_exception(e);
}
catch (std::bad_alloc const& ba) {
throw ba;
}
catch (...) {
throw;
}
}

Does 'Line 38' make sense?
That function seems to just rethrow exception_ptr.
As I think, 'Line 38' should be changed to

throw exception_list(std::current_exception());
@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 11, 2017

Member

@taeguk please construct a test which exposes the problem you believe we have.

Member

hkaiser commented Aug 11, 2017

@taeguk please construct a test which exposes the problem you believe we have.

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 12, 2017

Member

@hkaiser This is not bug which can be tested. This is the problem about intention of that function. And I want to just question about it.
I want to know what is the meaning of that function.
Is it intended that function just rethrows std::exception_ptr?

template <typename T>
static void call(std::vector<hpx::future<T> > const& workitems,
std::list<std::exception_ptr>& errors)
{
for (hpx::future<T> const& f: workitems)
{
if (f.has_exception())
call(f.get_exception_ptr(), errors);
}
if (!errors.empty())
throw exception_list(std::move(errors));
}

Like above, other overloaded functions of that function are related to exception_list.
So, I guess that 'Line 38' is an mistake and changing to throw exception_list(std::current_exception()); may be reasonable.
I can't determine whether my guessing is correct because that code is not written by me.

Member

taeguk commented Aug 12, 2017

@hkaiser This is not bug which can be tested. This is the problem about intention of that function. And I want to just question about it.
I want to know what is the meaning of that function.
Is it intended that function just rethrows std::exception_ptr?

template <typename T>
static void call(std::vector<hpx::future<T> > const& workitems,
std::list<std::exception_ptr>& errors)
{
for (hpx::future<T> const& f: workitems)
{
if (f.has_exception())
call(f.get_exception_ptr(), errors);
}
if (!errors.empty())
throw exception_list(std::move(errors));
}

Like above, other overloaded functions of that function are related to exception_list.
So, I guess that 'Line 38' is an mistake and changing to throw exception_list(std::current_exception()); may be reasonable.
I can't determine whether my guessing is correct because that code is not written by me.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 12, 2017

Member

@taeguk you're right, this is a nice catch. I agree this has to be changed as you suggest. But a test would be nice anyways, mostly to ensure that those functions throw an exception_list under any circumstances.

Member

hkaiser commented Aug 12, 2017

@taeguk you're right, this is a nice catch. I agree this has to be changed as you suggest. But a test would be nice anyways, mostly to ensure that those functions throw an exception_list under any circumstances.

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 12, 2017

Member

@hkaiser I want to change 'Line 38' to what I said.

Member

taeguk commented Aug 12, 2017

@hkaiser I want to change 'Line 38' to what I said.

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 14, 2017

Member

@hkaiser Do you agree that I change that?
With changing, handling exception for parallel algorithms can be abstracted as a function than explicitly coded. (https://github.com/taeguk/hpx/blob/f2d9cf60f692b50e81a69e78a93c54d0596a3294/hpx/parallel/algorithms/merge.hpp#L296-L304)

Member

taeguk commented Aug 14, 2017

@hkaiser Do you agree that I change that?
With changing, handling exception for parallel algorithms can be abstracted as a function than explicitly coded. (https://github.com/taeguk/hpx/blob/f2d9cf60f692b50e81a69e78a93c54d0596a3294/hpx/parallel/algorithms/merge.hpp#L296-L304)

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 14, 2017

Member

@taeguk, yes, please go ahead.

Member

hkaiser commented Aug 14, 2017

@taeguk, yes, please go ahead.

taeguk added a commit to taeguk/hpx that referenced this issue Aug 14, 2017

@taeguk taeguk changed the title from An strange thing in parallel::util::detail::handle_local_exceptions. to A strange thing in parallel::util::detail::handle_local_exceptions. Aug 14, 2017

@hkaiser hkaiser closed this in #2832 Aug 21, 2017

hkaiser added a commit that referenced this issue Aug 21, 2017

Merge pull request #2832 from taeguk/tg_fix_2818
Fix a strange thing in parallel::util::detail::handle_local_exceptions. (Fix #2818)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment