Merge pull request #9729 from chef/simpler_windows_timeout

Set timeouts in service and windows_service correctly

5 files changed, 23 insertions, 11 deletions

diff --git a/lib/chef/provider/service/windows.rb b/lib/chef/provider/service/windows.rb
index 63cc5fd26c..6c1f7c3583 100644
--- a/lib/chef/provider/service/windows.rb
+++ b/lib/chef/provider/service/windows.rb
@@ -47,8 +47,6 @@ class Chef::Provider::Service::Windows < Chef::Provider::Service
   START_PENDING = "start pending".freeze
   STOP_PENDING  = "stop pending".freeze
 
-  TIMEOUT = 60
-
   SERVICE_RIGHT = "SeServiceLogonRight".freeze
 
   def load_current_resource
@@ -329,22 +327,18 @@ class Chef::Provider::Service::Windows < Chef::Provider::Service
     retries = 0
     loop do
       break if current_state == desired_state
-      raise Timeout::Error if ( retries += 1 ) > resource_timeout
+      raise Timeout::Error if ( retries += 1 ) > @new_resource.timeout
 
       sleep 1
     end
   end
 
-  def resource_timeout
-    @resource_timeout ||= @new_resource.timeout || TIMEOUT
-  end
-
   def spawn_command_thread
     worker = Thread.new do
       yield
     end
 
-    Timeout.timeout(resource_timeout) do
+    Timeout.timeout(@new_resource.timeout) do
       worker.join
     end
   end
diff --git a/lib/chef/resource/service.rb b/lib/chef/resource/service.rb
index 50a7dda437..2a55973500 100644
--- a/lib/chef/resource/service.rb
+++ b/lib/chef/resource/service.rb
@@ -114,10 +114,10 @@ class Chef
       property :priority, [ Integer, String, Hash ],
         description: "Debian platform only. The relative priority of the program for start and shutdown ordering. May be an integer or a Hash. An integer is used to define the start run levels; stop run levels are then 100-integer. A Hash is used to define values for specific run levels. For example, { 2 => [:start, 20], 3 => [:stop, 55] } will set a priority of twenty for run level two and a priority of fifty-five for run level three."
 
-      # timeout only applies to the windows service manager
       property :timeout, Integer,
-        description: "Microsoft Windows platform only. The amount of time (in seconds) to wait before timing out.",
-        desired_state: false
+      description: "The amount of time (in seconds) to wait before timing out.",
+      default: 900,
+      desired_state: false
 
       property :parameters, Hash,
         description: "Upstart only: A hash of parameters to pass to the service command for use in the service definition."
diff --git a/lib/chef/resource/windows_service.rb b/lib/chef/resource/windows_service.rb
index 9d61990605..34f3cfb9a7 100644
--- a/lib/chef/resource/windows_service.rb
+++ b/lib/chef/resource/windows_service.rb
@@ -41,6 +41,11 @@ class Chef
 
       allowed_actions :configure_startup, :create, :delete, :configure
 
+      property :timeout, Integer,
+        description: "The amount of time (in seconds) to wait before timing out.",
+        default: 60,
+        desired_state: false
+
       # The display name to be used by user interface programs to identify the
       # service. This string has a maximum length of 256 characters.
       property :display_name, String, regex: /^.{1,256}$/,
diff --git a/spec/unit/resource/service_spec.rb b/spec/unit/resource/service_spec.rb
index 9bade1698d..a94beece6b 100644
--- a/spec/unit/resource/service_spec.rb
+++ b/spec/unit/resource/service_spec.rb
@@ -145,6 +145,10 @@ describe Chef::Resource::Service do
     expect(resource.timeout).to eql(1)
   end
 
+  it "defaults the timeout property to 900 (seconds)" do
+    expect(resource.timeout).to eql(900)
+  end
+
   %w{enabled running}.each do |prop|
     it "accepts true for #{prop} property" do
       resource.send(prop, true)
diff --git a/spec/unit/resource/windows_service_spec.rb b/spec/unit/resource/windows_service_spec.rb
index 4f93465b19..1a35e1ad52 100644
--- a/spec/unit/resource/windows_service_spec.rb
+++ b/spec/unit/resource/windows_service_spec.rb
@@ -48,6 +48,15 @@ describe Chef::Resource::WindowsService, "initialize" do
     expect { resource.action :unmask }.not_to raise_error
   end
 
+  it "accepts an Integer for timeout property" do
+    resource.timeout 1
+    expect(resource.timeout).to eql(1)
+  end
+
+  it "defaults the timeout property to 60 (seconds)" do
+    expect(resource.timeout).to eql(60)
+  end
+
   %i{automatic manual disabled}.each do |type|
     it "supports setting startup_type property to #{type.inspect}" do
       resource.startup_type type