[future] Make new IValue future constValue semantics match torch::utils counterpart (#38355)

Jeremy Lilley · facebook-github-bot · commit 328dd9e5d61d · 2020-05-13T15:02:23.000-07:00
Summary: Pull Request resolved: #38355 The torch::utils::Future api which this api was copied from last week intentionally does not throw. Harmonize the semantics and comment appropriately. ghstack-source-id: 104014210 Test Plan: buck test mode/dev-nosan caffe2/test/... Differential Revision: D21533016 fbshipit-source-id: db26af32656d7b9dacf4fad4e77c944a0087c9b0
diff --git a/aten/src/ATen/core/ivalue_inl.h b/aten/src/ATen/core/ivalue_inl.h
@@ -310,12 +310,11 @@ struct C10_EXPORT ivalue::Future final : c10::intrusive_ptr_target {
     return value_;
   }
 
+  // This accessor should only be used if we know that the future is
+  // completed() with no error.
   const IValue& constValue() {
     std::unique_lock<std::mutex> lock(mutex_);
     AT_ASSERT(completed());
-    if (error_) {
-      throw *error_;
-    }
     return value_;
   }
 
diff --git a/torch/csrc/distributed/rpc/rref_impl.cpp b/torch/csrc/distributed/rpc/rref_impl.cpp
@@ -203,6 +203,9 @@ RRefForkData UserRRef::fork() const {
 
 const IValue& OwnerRRef::getValue() const {
   future_->wait();
+  if (future_->hasError()) {
+    (void)future_->value(); // Throws the error.
+  }
   return future_->constValue();
 }
 
diff --git a/torch/csrc/utils/future.h b/torch/csrc/utils/future.h
@@ -47,6 +47,8 @@ class TORCH_API Future final {
     return value_;
   }
 
+  // These constValue/moveValue accessors should only be used if
+  // we know that the future is completed() with no error.
   const T& constValue() const {
     std::unique_lock<std::mutex> lock(mutex_);
     AT_ASSERT(completed_);

Original file line number	Diff line number	Diff line change
`@@ -310,12 +310,11 @@ struct C10_EXPORT ivalue::Future final : c10::intrusive_ptr_target {`
`310`	`310`	`return value_;`
`311`	`311`	`}`
`312`	`312`
	`313`	`+ // This accessor should only be used if we know that the future is`
	`314`	`+ // completed() with no error.`
`313`	`315`	`const IValue& constValue() {`
`314`	`316`	`std::unique_lock<std::mutex> lock(mutex_);`
`315`	`317`	`AT_ASSERT(completed());`
`316`		`- if (error_) {`
`317`		`- throw *error_;`
`318`		`- }`
`319`	`318`	`return value_;`
`320`	`319`	`}`
`321`	`320`
Original file line number	Diff line number	Diff line change
`@@ -203,6 +203,9 @@ RRefForkData UserRRef::fork() const {`
`203`	`203`
`204`	`204`	`const IValue& OwnerRRef::getValue() const {`
`205`	`205`	`future_->wait();`
	`206`	`+ if (future_->hasError()) {`
	`207`	`+ (void)future_->value(); // Throws the error.`
	`208`	`+ }`
`206`	`209`	`return future_->constValue();`
`207`	`210`	`}`
`208`	`211`
Original file line number	Diff line number	Diff line change
`@@ -47,6 +47,8 @@ class TORCH_API Future final {`
`47`	`47`	`return value_;`
`48`	`48`	`}`
`49`	`49`
	`50`	`+ // These constValue/moveValue accessors should only be used if`
	`51`	`+ // we know that the future is completed() with no error.`
`50`	`52`	`const T& constValue() const {`
`51`	`53`	`std::unique_lock<std::mutex> lock(mutex_);`
`52`	`54`	`AT_ASSERT(completed_);`