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

One suspicion in parallel::detail::handle_exception. #2834

Closed
taeguk opened this Issue Aug 15, 2017 · 4 comments

Comments

3 participants
@taeguk
Member

taeguk commented Aug 15, 2017

I have one doubt about parallel::detail::handle_exception.

template <typename ExPolicy, typename Result = void>
struct handle_exception_impl
{
typedef Result type;
HPX_ATTRIBUTE_NORETURN static Result call()
{
try {
throw; //-V667
}
catch(std::bad_alloc const& e) {
throw e;
}
catch (hpx::exception_list const& el) {
throw el;
}
catch (...) {
throw hpx::exception_list(std::current_exception());
}
}
static hpx::future<Result> call(hpx::future<Result> f)
{
HPX_ASSERT(f.has_exception());
// Intel complains if this is not explicitly moved
return std::move(f);
}
static hpx::future<Result> call(std::exception_ptr const& e)
{
try {
std::rethrow_exception(e);
}
catch (std::bad_alloc const&) {
// rethrow bad_alloc
return hpx::make_exceptional_future<Result>(
std::current_exception());
}
catch (hpx::exception_list const& el) {
// rethrow exception_list
return hpx::make_exceptional_future<Result>(el);
}
catch (...) {
// package up everything else as an exception_list
return hpx::make_exceptional_future<Result>(
exception_list(e));
}
}

Above, the first handle_exception_impl<>::call() in Line 31 just throws exception. But, the second and third one return an exceptional future. As I think, this is strange. It seems inconsistent.

I think that the struct of Line 79 is for parallel algorithm called with execution::task. So, in the struct, call() all both should return hpx::future<Result>. (In other words, should return exceptional future.)
But in case of Line 27, it is different case. So, it should just return Result as I think. (In other words, should just throw exception.)

How do you think about this issue?

@hkaiser hkaiser added this to the 1.1.0 milestone Aug 15, 2017

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 15, 2017

Member

This looks suspicious, yes. I will try to investigate in more detail over the next days as I have no immediate answer to why we have it this way. Since all tests pass I'd suspect that

  • we either don't test all possible exceptional cases and the code is incorrect,
  • the code in question above is never called,
  • or simply the above code is correct
Member

hkaiser commented Aug 15, 2017

This looks suspicious, yes. I will try to investigate in more detail over the next days as I have no immediate answer to why we have it this way. Since all tests pass I'd suspect that

  • we either don't test all possible exceptional cases and the code is incorrect,
  • the code in question above is never called,
  • or simply the above code is correct

@msimberg msimberg removed this from the 1.1.0 milestone Dec 5, 2017

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Dec 31, 2017

Member

@taeguk has this been fixed by now?

Member

hkaiser commented Dec 31, 2017

@taeguk has this been fixed by now?

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Jan 1, 2018

Member

@hkaiser This has been not fixed by now. I thought that this is still suspicious. And I waited your reply until now.
How we go about this? Fixing this suspicious thing as I suggest? Or what should we do?

Member

taeguk commented Jan 1, 2018

@hkaiser This has been not fixed by now. I thought that this is still suspicious. And I waited your reply until now.
How we go about this? Fixing this suspicious thing as I suggest? Or what should we do?

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jan 1, 2018

Member

@taeguk I'd appreciate it for you to come up with a fix for this. Thanks!

Member

hkaiser commented Jan 1, 2018

@taeguk I'd appreciate it for you to come up with a fix for this. Thanks!

taeguk added a commit to taeguk/hpx that referenced this issue Jan 1, 2018

Fix a strange thing in parallel::detail::handle_exception.
There are some changes in parallel::sort for fixing STEllAR-GROUP#2834.
And this commit also fixes STEllAR-GROUP#3088.

@hkaiser hkaiser closed this in #3089 Jan 8, 2018

hkaiser added a commit that referenced this issue Jan 8, 2018

Merge pull request #3089 from taeguk/tg_fix_2834
Fix a strange thing in parallel::detail::handle_exception. (Fix #2834.)

@hkaiser hkaiser added this to Merged to master in Standard Algorithms Jan 21, 2018

@msimberg msimberg added this to the 1.1.0 milestone Mar 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment