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

The std::bad_alloc is not handled correctly in parallel::sort. #2816

Open
taeguk opened this Issue Aug 11, 2017 · 8 comments

Comments

3 participants
@taeguk
Member

taeguk commented Aug 11, 2017

The std::bad_alloc seems be not handled correctly in parallel::sort.

return hpx::dataflow(
[last](hpx::future<RandomIt> && left,
hpx::future<RandomIt> && right) -> RandomIt
{
if (left.has_exception() || right.has_exception())
{
std::list<std::exception_ptr> errors;
if (left.has_exception())
errors.push_back(left.get_exception_ptr());
if (right.has_exception())
errors.push_back(right.get_exception_ptr());
throw exception_list(std::move(errors));
}
return last;
},
std::move(left), std::move(right));

As I know, std::bad_alloc should be thrown itself. But, by above codes, std::bad_alloc is wrapped into exception_list.

As I think, using util::detail::handle_local_exception is better way like this:

std::list<std::exception_ptr> errors;
// TODO: Is it okay to use thing in util::detail:: ?
util::detail::handle_local_exceptions<ExPolicy>::call(
remaining_block_futures, errors);

Maybe there are same problems in other parallel algorithms.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 11, 2017

Member

@taeguk: please construct a test exposing the issue. We'll take it from there.

Member

hkaiser commented Aug 11, 2017

@taeguk: please construct a test exposing the issue. We'll take it from there.

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 11, 2017

Member

@hkaiser Below, Can this be an evidence for this issue?
taeguk@b846ca7

Member

taeguk commented Aug 11, 2017

@hkaiser Below, Can this be an evidence for this issue?
taeguk@b846ca7

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 12, 2017

Member

@taeguk, looks good, could you separate this into a separate test here, please?

Member

hkaiser commented Aug 12, 2017

@taeguk, looks good, could you separate this into a separate test here, please?

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 12, 2017

Member

@hkaiser As I think, adding my test code to there is strange because it is the code which has a bug on purpose. My test code will always fails. But normally, test is for not failing but success. So, inserting failed code to test folder can be confusing. The person who sees the test can misthink the test code doesn't have a bug.

Member

taeguk commented Aug 12, 2017

@hkaiser As I think, adding my test code to there is strange because it is the code which has a bug on purpose. My test code will always fails. But normally, test is for not failing but success. So, inserting failed code to test folder can be confusing. The person who sees the test can misthink the test code doesn't have a bug.

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Aug 12, 2017

Member

@taeguk I really believe that having a test verifying that we have fixed the issue is needed. It will just force us to come up with a fix if we add it now and it fails. For each (significant) fix we usually add a corresponding test which makes sure that the problem is gone and stays gone.

Member

hkaiser commented Aug 12, 2017

@taeguk I really believe that having a test verifying that we have fixed the issue is needed. It will just force us to come up with a fix if we add it now and it fails. For each (significant) fix we usually add a corresponding test which makes sure that the problem is gone and stays gone.

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Aug 12, 2017

Member

@hkaiser Yes, you're right. But the problem is that test code will always fail without modification of test code itself. For resolving this issue, we should modify the test code to use parallel::util::detail::handle_local_exception like https://github.com/taeguk/hpx/blob/9cc2753151706e484b806fbf894e6b8fc4c113fa/hpx/parallel/algorithms/merge.hpp#L228-L237

Member

taeguk commented Aug 12, 2017

@hkaiser Yes, you're right. But the problem is that test code will always fail without modification of test code itself. For resolving this issue, we should modify the test code to use parallel::util::detail::handle_local_exception like https://github.com/taeguk/hpx/blob/9cc2753151706e484b806fbf894e6b8fc4c113fa/hpx/parallel/algorithms/merge.hpp#L228-L237

@hkaiser hkaiser added this to Open Tickets in Standard Algorithms Jan 21, 2018

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jan 21, 2018

Member

@taeguk I think this can be closed now, is that correct?

Member

hkaiser commented Jan 21, 2018

@taeguk I think this can be closed now, is that correct?

@taeguk

This comment has been minimized.

Show comment
Hide comment
@taeguk

taeguk Jan 22, 2018

Member

@hkaiser No, it is not resolved yet.

Member

taeguk commented Jan 22, 2018

@hkaiser No, it is not resolved yet.

@msimberg msimberg removed this from the 1.1.0 milestone Mar 22, 2018

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