Inhibit direct conversion from future<future<T>> --> future<void> #2673
Conversation
|
@dmarce1 Please check this out and let us know if this fixes the issue you're seeing. |
| }); | ||
|
|
||
| hpx::future<void> fut2 = std::move(fut); | ||
| fut2.get(); |
There was a problem hiding this comment.
Shouldn't we test here that the inner async has been executed after fut2.get() succeeded?
There was a problem hiding this comment.
This is a test making sure that things don't compile. No testing of flags would make a difference.
There was a problem hiding this comment.
ahh, missed that this is just a compile only test ... giving essentially the same test below, is a separate, compile only test really necessary? The test will fail if it does not compile.
There was a problem hiding this comment.
It's not the same test. The compile-fail test makes sure the conversion future<future<T>> -> future<void> does not compile (T != void), while the other test makes sure the conversion future<future<void>> -> future<void> is doing what it is supposed to do, namely unwrapping the nested future.
| hpx::future<void> fut2 = std::move(fut); | ||
| fut2.get(); | ||
|
|
||
| HPX_TEST(t.elapsed() > 1.0); |
There was a problem hiding this comment.
Is checking the elapsed time the right thing to do or shouldn't we rather check if some flag or similar has been set?
There was a problem hiding this comment.
Yes, checking a flag can be done additionally. The time check is supposed to make sure that the collapsed future becomes ready only after the inner future.
There was a problem hiding this comment.
@sithhell I now have added the additional check as you suggested.
- adding corresponding tests
|
@sithhell your comments have been addressed. |
There was a problem hiding this comment.
The bug I am seeing turns up with extreme irregularity. (I just ran for 2 days on 64 nodes without it appearing) I also have no way of knowing for sure (at this point) the problem addressed by this pull request is what is causing the bug. I was unable to reproduce the issue with a smaller test case. At this point I think we should merge and will recompile and rerun and wait and see.
|
@dmarce1 Thanks. We know for sure that this PR addresses a problem, it might not be the problem you were seeing. With this PR, did you see compilation errors with octotiger? Did you have to change anything? |
|
@hkaiser No compilation errors, didn't have to change anything. Everything appears to be working. |
@dmarce1: ok, then you're most likely not affected by the problem this PR solves. |
adding corresponding tests
this fixes Ambiguity of nested hpx::future<void>'s. #2667