Merge pull request #1108 from yang-g/overflowfix

Time point conversion fix

build.json

@@ -1707,6 +1707,21 @@
         "gpr"
       ]
     },
+    {
+      "name": "cxx_time_test",
+      "build": "test",
+      "language": "c++",
+      "src": [
+        "test/cpp/util/time_test.cc"
+      ],
+      "deps": [
+        "grpc_test_util",
+        "grpc++",
+        "grpc",
+        "gpr_test_util",
+        "gpr"
+      ]
+    },
     {
       "name": "end2end_test",
       "build": "test",

include/grpc++/completion_queue.h

@@ -36,6 +36,7 @@
 
 #include <chrono>
 #include <grpc++/impl/client_unary_call.h>
+#include <grpc/support/time.h>
 
 struct grpc_completion_queue;
 
@@ -88,9 +89,7 @@ class CompletionQueue {
   // Returns false if the queue is ready for destruction, true if event
 
   bool Next(void** tag, bool* ok) {
-    return (
-        AsyncNext(tag, ok, (std::chrono::system_clock::time_point::max)()) !=
-        SHUTDOWN);
+    return (AsyncNextInternal(tag, ok, gpr_inf_future) != SHUTDOWN);
   }
 
   // Shutdown has to be called, and the CompletionQueue can only be
@@ -122,6 +121,8 @@ class CompletionQueue {
                                   const grpc::protobuf::Message& request,
                                   grpc::protobuf::Message* result);
 
+  NextStatus AsyncNextInternal(void** tag, bool* ok, gpr_timespec deadline);
+
   // Wraps grpc_completion_queue_pluck.
   // Cannot be mixed with calls to Next().
   bool Pluck(CompletionQueueTag* tag);

src/cpp/common/completion_queue.cc

@@ -36,7 +36,6 @@
 
 #include <grpc/grpc.h>
 #include <grpc/support/log.h>
-#include <grpc/support/time.h>
 #include "src/cpp/util/time.h"
 
 namespace grpc {
@@ -57,15 +56,12 @@ class EventDeleter {
   }
 };
 
-CompletionQueue::NextStatus
-CompletionQueue::AsyncNext(void** tag, bool* ok,
-			   std::chrono::system_clock::time_point deadline) {
+CompletionQueue::NextStatus CompletionQueue::AsyncNextInternal(
+    void** tag, bool* ok, gpr_timespec deadline) {
   std::unique_ptr<grpc_event, EventDeleter> ev;
 
-  gpr_timespec gpr_deadline;
-  Timepoint2Timespec(deadline, &gpr_deadline);
   for (;;) {
-    ev.reset(grpc_completion_queue_next(cq_, gpr_deadline));
+    ev.reset(grpc_completion_queue_next(cq_, deadline));
     if (!ev) { /* got a NULL back because deadline passed */
       return TIMEOUT;
     }
@@ -81,6 +77,13 @@ CompletionQueue::AsyncNext(void** tag, bool* ok,
   }
 }
 
+CompletionQueue::NextStatus CompletionQueue::AsyncNext(
+    void** tag, bool* ok, std::chrono::system_clock::time_point deadline) {
+  gpr_timespec gpr_deadline;
+  Timepoint2Timespec(deadline, &gpr_deadline);
+  return AsyncNextInternal(tag, ok, gpr_deadline);
+}
+
 bool CompletionQueue::Pluck(CompletionQueueTag* tag) {
   std::unique_ptr<grpc_event, EventDeleter> ev;
 

src/cpp/util/time.cc

@@ -42,11 +42,15 @@ using std::chrono::system_clock;
 
 namespace grpc {
 
-// TODO(yangg) prevent potential overflow.
 void Timepoint2Timespec(const system_clock::time_point& from,
                         gpr_timespec* to) {
   system_clock::duration deadline = from.time_since_epoch();
   seconds secs = duration_cast<seconds>(deadline);
+  if (from == system_clock::time_point::max() ||
+      secs.count() >= gpr_inf_future.tv_sec || secs.count() < 0) {
+    *to = gpr_inf_future;
+    return;
+  }
   nanoseconds nsecs = duration_cast<nanoseconds>(deadline - secs);
   to->tv_sec = secs.count();
   to->tv_nsec = nsecs.count();

test/cpp/util/time_test.cc

@@ -61,11 +61,24 @@ TEST_F(TimeTest, AbsolutePointTest) {
   EXPECT_TRUE(tp == tp_converted_2);
 }
 
-// gpr_inf_future is treated specially and mapped to time_point::max()
+// gpr_inf_future is treated specially and mapped to/from time_point::max()
 TEST_F(TimeTest, InfFuture) {
   EXPECT_EQ(system_clock::time_point::max(),
             Timespec2Timepoint(gpr_inf_future));
+  gpr_timespec from_time_point_max;
+  Timepoint2Timespec(system_clock::time_point::max(), &from_time_point_max);
+  EXPECT_EQ(0, gpr_time_cmp(gpr_inf_future, from_time_point_max));
+  // This will cause an overflow
+  Timepoint2Timespec(
+      std::chrono::time_point<system_clock, std::chrono::seconds>::max(),
+      &from_time_point_max);
+  EXPECT_EQ(0, gpr_time_cmp(gpr_inf_future, from_time_point_max));
 }
 
 }  // namespace
 }  // namespace grpc
+
+int main(int argc, char** argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}

tools/run_tests/tests.json

@@ -351,6 +351,11 @@
     "language": "c++", 
     "name": "credentials_test"
   }, 
+  {
+    "flaky": false, 
+    "language": "c++", 
+    "name": "cxx_time_test"
+  }, 
   {
     "flaky": false, 
     "language": "c++",