Commit 547f5abf authored by Constantin Dolghier's avatar Constantin Dolghier Committed by Facebook Github Bot

Make semantically-const methods in SharedPromise const

Summary:
1) SharedPromise is semantically a regular Promise one can get multiple Futures off. One would expect that all methods which are const in Promise to also be const in SharedPromise - but they are not because their implementations needs using a mutex.
2) Semantically, `SharedPromise::getFuture()` and `SharedPromise::getSemiFuture()` do not change the observable state of the object.
3) As real-life consequence,  these non-const `SharedPromise` methods become viral in downstream code which then has to hack around it in downstream `const` methods by marking the SharedPromise instance mutable. That solution leads to the program being less const-correct than it should be, as now the type system allows `const` methods to reassign or `setValue` the SharedPromise.

This diff changes `hasResult`, `isFulfilled`, `size()`, `getFuture()` and `getSemiFuture()` to be `const`. In order to achieve that, the private fields `mutex_` and `size_` had to be marked `mutable`.

(Note: this ignores all push blocking failures!)

Reviewed By: yfeldblum

Differential Revision: D18565877

fbshipit-source-id: ff63a48c02235f72c5b0191423ae68f7b7c42fb3
parent c2bab5e5
...@@ -19,13 +19,13 @@ ...@@ -19,13 +19,13 @@
namespace folly { namespace folly {
template <class T> template <class T>
size_t SharedPromise<T>::size() { size_t SharedPromise<T>::size() const {
std::lock_guard<std::mutex> g(mutex_); std::lock_guard<std::mutex> g(mutex_);
return size_.value; return size_.value;
} }
template <class T> template <class T>
SemiFuture<T> SharedPromise<T>::getSemiFuture() { SemiFuture<T> SharedPromise<T>::getSemiFuture() const {
std::lock_guard<std::mutex> g(mutex_); std::lock_guard<std::mutex> g(mutex_);
size_.value++; size_.value++;
if (hasResult()) { if (hasResult()) {
...@@ -40,7 +40,7 @@ SemiFuture<T> SharedPromise<T>::getSemiFuture() { ...@@ -40,7 +40,7 @@ SemiFuture<T> SharedPromise<T>::getSemiFuture() {
} }
template <class T> template <class T>
Future<T> SharedPromise<T>::getFuture() { Future<T> SharedPromise<T>::getFuture() const {
return getSemiFuture().via(&InlineExecutor::instance()); return getSemiFuture().via(&InlineExecutor::instance());
} }
...@@ -100,7 +100,7 @@ void SharedPromise<T>::setTry(Try<T>&& t) { ...@@ -100,7 +100,7 @@ void SharedPromise<T>::setTry(Try<T>&& t) {
} }
template <class T> template <class T>
bool SharedPromise<T>::isFulfilled() { bool SharedPromise<T>::isFulfilled() const {
std::lock_guard<std::mutex> g(mutex_); std::lock_guard<std::mutex> g(mutex_);
return hasResult(); return hasResult();
} }
......
...@@ -44,7 +44,7 @@ class SharedPromise { ...@@ -44,7 +44,7 @@ class SharedPromise {
* Return a Future tied to the shared core state. Unlike Promise::getFuture, * Return a Future tied to the shared core state. Unlike Promise::getFuture,
* this can be called an unlimited number of times per SharedPromise. * this can be called an unlimited number of times per SharedPromise.
*/ */
SemiFuture<T> getSemiFuture(); SemiFuture<T> getSemiFuture() const;
/** /**
* Return a Future tied to the shared core state. Unlike Promise::getFuture, * Return a Future tied to the shared core state. Unlike Promise::getFuture,
...@@ -53,10 +53,10 @@ class SharedPromise { ...@@ -53,10 +53,10 @@ class SharedPromise {
* appropriate executor to .via on the returned SemiFuture to get a * appropriate executor to .via on the returned SemiFuture to get a
* valid Future where necessary. * valid Future where necessary.
*/ */
Future<T> getFuture(); Future<T> getFuture() const;
/** Return the number of Futures associated with this SharedPromise */ /** Return the number of Futures associated with this SharedPromise */
size_t size(); size_t size() const;
/** Fulfill the SharedPromise with an exception_wrapper */ /** Fulfill the SharedPromise with an exception_wrapper */
void setException(exception_wrapper ew); void setException(exception_wrapper ew);
...@@ -97,7 +97,7 @@ class SharedPromise { ...@@ -97,7 +97,7 @@ class SharedPromise {
template <class F> template <class F>
void setWith(F&& func); void setWith(F&& func);
bool isFulfilled(); bool isFulfilled() const;
private: private:
// this allows SharedPromise move-ctor/move-assign to be defaulted // this allows SharedPromise move-ctor/move-assign to be defaulted
...@@ -125,14 +125,14 @@ class SharedPromise { ...@@ -125,14 +125,14 @@ class SharedPromise {
} }
}; };
bool hasResult() { bool hasResult() const {
return try_.value.hasValue() || try_.value.hasException(); return try_.value.hasValue() || try_.value.hasException();
} }
Mutex mutex_; mutable Mutex mutex_;
Defaulted<size_t> size_; mutable Defaulted<size_t> size_;
Defaulted<Try<T>> try_; Defaulted<Try<T>> try_;
std::vector<Promise<T>> promises_; mutable std::vector<Promise<T>> promises_;
std::function<void(exception_wrapper const&)> interruptHandler_; std::function<void(exception_wrapper const&)> interruptHandler_;
}; };
......
...@@ -146,3 +146,13 @@ TEST(SharedPromise, interruptHandler) { ...@@ -146,3 +146,13 @@ TEST(SharedPromise, interruptHandler) {
f.cancel(); f.cancel();
EXPECT_TRUE(flag); EXPECT_TRUE(flag);
} }
TEST(SharedPromise, ConstMethods) {
SharedPromise<int> p;
EXPECT_FALSE(folly::as_const(p).isFulfilled());
auto fut = folly::as_const(p).getFuture();
EXPECT_FALSE(fut.isReady());
p.setValue(42);
EXPECT_TRUE(fut.isReady());
EXPECT_EQ(42, std::move(fut).get());
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment