Treat zero --duration flag as infine

With this, --duration 0 will be treated as a request to infinitely
execute. Other default predicates will still exist, to execution
may stop when connection failures or error response codes are
observed.

This leaves some small tech debt:

- We shouldn't need to create a foo root termination predicate
- We need test coverage, but that's much to add once
  envoyproxy#280 or envoyproxy#344 goes in

Fixes envoyproxy#333

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

README.md

@@ -218,7 +218,8 @@ Use HTTP/2
 Connection connect timeout period in seconds. Default: 30.
 
 --duration <uint32_t>
-The number of seconds that the test should run. Default: 5.
+The number of seconds that the test should run. Default: 5. 0 will be
+treated as infinite.
 
 --connections <uint32_t>
 The maximum allowed number of concurrent connections per event loop.

api/client/options.proto

@@ -102,8 +102,8 @@ message CommandLineOptions {
       [(validate.rules).uint32 = {gte: 1, lte: 1000000}];
   // The maximum allowed number of concurrent connections per event loop. HTTP/1 only. Default: 100.
   google.protobuf.UInt32Value connections = 2 [(validate.rules).uint32 = {gte: 1, lte: 1000000}];
-  // The number of seconds that the test should run. Default: 5.
-  google.protobuf.Duration duration = 3 [(validate.rules).duration.gte.nanos = 1];
+  // The number of seconds that the test should run. Default: 5. 0 will be treated as infinite.
+  google.protobuf.Duration duration = 3 [(validate.rules).duration.gte.nanos = 0];
   // Connection connect timeout period in seconds. Default: 30.
   google.protobuf.Duration timeout = 4 [(validate.rules).duration.gte.seconds = 0];
   // Use HTTP/2

source/client/factories_impl.cc

@@ -17,6 +17,8 @@
 #include "client/output_collector_impl.h"
 #include "client/output_formatter_impl.h"
 
+using namespace std::chrono_literals;
+
 namespace Nighthawk {
 namespace Client {
 
@@ -172,15 +174,24 @@ TerminationPredicateFactoryImpl::TerminationPredicateFactoryImpl(const Options&
 TerminationPredicatePtr
 TerminationPredicateFactoryImpl::create(Envoy::TimeSource& time_source, Envoy::Stats::Scope& scope,
                                         const Envoy::MonotonicTime scheduled_starting_time) const {
-  TerminationPredicatePtr duration_predicate = std::make_unique<DurationTerminationPredicateImpl>(
-      time_source, options_.duration(), scheduled_starting_time);
-  TerminationPredicate* current_predicate = duration_predicate.get();
+  TerminationPredicatePtr root_predicate;
+  if (options_.duration() > 0s) {
+    root_predicate = std::make_unique<DurationTerminationPredicateImpl>(
+        time_source, options_.duration(), scheduled_starting_time);
+  } else {
+    // TODO(oschaaf): clean this up. Refactor linkConfiguredPredicates so we can easily omit
+    // this.
+    root_predicate = std::make_unique<StatsCounterAbsoluteThresholdTerminationPredicateImpl>(
+        scope.counterFromString("nonexisting.counter"), 1, TerminationPredicate::Status::FAIL);
+  }
+
+  TerminationPredicate* current_predicate = root_predicate.get();
   current_predicate = linkConfiguredPredicates(*current_predicate, options_.failurePredicates(),
                                                TerminationPredicate::Status::FAIL, scope);
   linkConfiguredPredicates(*current_predicate, options_.terminationPredicates(),
                            TerminationPredicate::Status::TERMINATE, scope);
 
-  return duration_predicate;
+  return root_predicate;
 }
 
 TerminationPredicate* TerminationPredicateFactoryImpl::linkConfiguredPredicates(

source/client/options_impl.cc

@@ -46,10 +46,11 @@ OptionsImpl::OptionsImpl(int argc, const char* const* argv) {
                   "only. Default: {}.",
                   connections_),
       false, 0, "uint32_t", cmd);
-  TCLAP::ValueArg<uint32_t> duration(
-      "", "duration",
-      fmt::format("The number of seconds that the test should run. Default: {}.", duration_), false,
-      0, "uint32_t", cmd);
+  TCLAP::ValueArg<uint32_t> duration("", "duration",
+                                     fmt::format("The number of seconds that the test should run. "
+                                                 "Default: {}. 0 will be treated as infinite.",
+                                                 duration_),
+                                     false, 0, "uint32_t", cmd);
   TCLAP::ValueArg<uint32_t> timeout(
       "", "timeout",
       fmt::format("Connection connect timeout period in seconds. Default: {}.", timeout_), false, 0,

source/common/termination_predicate_impl.cc

@@ -16,7 +16,7 @@ TerminationPredicate::Status TerminationPredicateBaseImpl::evaluateChain() {
 }
 
 TerminationPredicate::Status DurationTerminationPredicateImpl::evaluate() {
-  return time_source_.monotonicTime() - start_ > duration_ ? TerminationPredicate::Status::TERMINATE
+  return (time_source_.monotonicTime() - start_) > duration_ ? TerminationPredicate::Status::TERMINATE
                                                            : TerminationPredicate::Status::PROCEED;
 }
 

test/options_test.cc

@@ -345,7 +345,7 @@ TEST_P(OptionsImplIntTestNonZeroable, NonZeroableOptions) {
 }
 
 INSTANTIATE_TEST_SUITE_P(NonZeroableIntOptionTests, OptionsImplIntTestNonZeroable,
-                         Values("rps", "connections", "duration", "max-active-requests",
+                         Values("rps", "connections", "max-active-requests",
                                 "max-requests-per-connection"));
 
 // Check standard expectations for any integer values options we offer.