-
Notifications
You must be signed in to change notification settings - Fork 12
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
EAC-4613 Add observe as alternative to then #19
Conversation
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.
LGTM! Mostly typos :)
std::forward<TFunc>(cont)); | ||
} | ||
|
||
//! \brief Observes the input futures and calls the given contionuation function once |
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.
Typo: contionuation
std::forward<TFunc>(cont))); | ||
} | ||
|
||
//! \brief Observes the input futures and calls the given contionuation function once |
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.
Typo: contionuation
namespace thousandeyes { | ||
namespace futures { | ||
|
||
//! \brief Observes the input futures and calls the given contionuation function once |
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.
Typo: contionuation
std::forward<TFunc>(cont)); | ||
} | ||
|
||
//! \brief Observes the input futures and calls the given contionuation function once |
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.
Typo
tests/defaultexecutor.cpp
Outdated
EXPECT_DEATH( | ||
{ | ||
observe(getExceptionAsync<SomeKindOfError>(), [&](future<void> f) { | ||
f.get(); |
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.
Nit: I think that by EXPECT_DEATH
you're testing more things that you have to. Isn't it enough to test EXPECT_THROW(f.get(), ExceptionType)
?
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.
I wanted to capture the fact that the exception is propagated into the dispatcher thread. If we're fine with testing only the fact that observe
calls the continuation, then indeed EXPECT_THROW
is enough.
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.
It's fine to leave as is. Only, it looks more like an integration test than a unit test.
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.
LGTM - some minor suggestions.
#pragma once | ||
|
||
#include <future> | ||
#include <memory> |
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.
Nit: memory is unused but chrono is used, I'd swap this for a chrono include
tests/defaultexecutor.cpp
Outdated
// Because f.get() throws, the remaining code in this block | ||
// is never executed. |
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.
If we expect to never get to this code, why not add a FAIL() << "Should not be executed";
line to incidate the issue to the person running the test?
mutex mtx; | ||
condition_variable cv; | ||
|
||
// This will never become true, as the continuation will rethrow the exception |
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.
Can we add a EXPECT_FALSE(success);
to the end of the test to make this expectation checked?
Different from then(), observe() returns void and will call the continuation on the executor thread. Any exceptions thrown by the continuation will therefore affect the executor's dispatch thread. It's intended use case is fire-and-forget futures that we still want to .get() to not silently swallow any exceptions.
Different from then(), observe() returns void and will call the continuation on the executor thread. Any exceptions thrown by the continuation will therefore affect the executor's dispatch thread.
It's intended use case is fire-and-forget futures that we still want to .get() to not silently swallow any exceptions.