From b8db256080b7a7f138b1e2ab13c5e4c25375fe78 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 15 Feb 2019 09:49:09 +0800
Subject: [PATCH] (MODULES-8358) Chunk read the pipe from the PowerShell
 Process

Previously the PowerShell Manager would attempt all the expected bytes from
the inbound pipe.  Normally this works fine.  However the Travis CI testing
was intermittently failing where the pipe was only reading half the pipe,
which then caused the first test to fail, and then any subsequent tests failed
because it was reading left-over data from the previous failed read.

The documentation of the IO.sysread method [1] says the argument is maxlen
which implies that this the maximum number of bytes it would return.  Note that
other methods in IO use the language "Reads at most maxlen bytes from ios"
whereas sysread uses the langauge "Reads maxlen bytes from ios"

This commit changes the pipe reader to read in chunks of bytes until all of
the expected bytes are read.  Note that this is a blocking call so if you
try to read beyond the bytes in the pipe it blocks the thread.  The value
of 8K chunk was an arbitrary number which is big enough to get most powershell
repsonse in a single IO read.  The integration tests were failing to read in
a 96K chunk (only ~36K were read)

[1] https://ruby-doc.org/core-2.6.1/IO.html#method-i-sysread
---
 .../puppetlabs/powershell/powershell_manager.rb    | 14 +++++++++++---
 .../puppet_x/puppetlabs/powershell_manager_spec.rb |  8 +++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/lib/puppet_x/puppetlabs/powershell/powershell_manager.rb b/lib/puppet_x/puppetlabs/powershell/powershell_manager.rb
index 470c76d1..e2de0f3f 100644
--- a/lib/puppet_x/puppetlabs/powershell/powershell_manager.rb
+++ b/lib/puppet_x/puppetlabs/powershell/powershell_manager.rb
@@ -394,10 +394,18 @@ def read_streams
         pipe_reader = Thread.new(@pipe) do |pipe|
           # read a Little Endian 32-bit integer for length of response
           expected_response_length = pipe.sysread(4).unpack('V').first
-          return nil if expected_response_length == 0
 
-          # reads the expected bytes as a binary string or fails
-          pipe.sysread(expected_response_length)
+          if expected_response_length == 0
+            nil
+          else
+            # reads the expected bytes as a binary string or fails
+            buffer = ""
+            # Reads in the pipe data 8K, or less, at a time
+            while (buffer.length < expected_response_length)
+              buffer << pipe.sysread([expected_response_length - buffer.length, 8192].min)
+            end
+            buffer
+          end
         end
 
         Puppet.debug "Waited #{Time.now - start_time} total seconds."
diff --git a/spec/integration/puppet_x/puppetlabs/powershell_manager_spec.rb b/spec/integration/puppet_x/puppetlabs/powershell_manager_spec.rb
index a347ea39..87b03277 100644
--- a/spec/integration/puppet_x/puppetlabs/powershell_manager_spec.rb
+++ b/spec/integration/puppet_x/puppetlabs/powershell_manager_spec.rb
@@ -559,7 +559,8 @@ def output_cmdlet(ps_command, ps_args)
 
     it "should be able to write more than the 64k default buffer size to the managers pipe without deadlocking the Ruby parent process or breaking the pipe" do
       # this was tested successfully up to 5MB of text
-      buffer_string_96k = 'a' * ((1024 * 96) + 1)
+      # we add some additional bytes so it's not always on a 1KB boundary and forces pipe reading in different lengths, not always 1K chunks
+      buffer_string_96k = 'a' * ((1024 * 96) + 11)
       result = manager.execute(<<-CODE
 '#{buffer_string_96k}' | #{output_cmdlet(ps_command, ps_args)}
         CODE
@@ -572,15 +573,16 @@ def output_cmdlet(ps_command, ps_args)
     end
 
     it "should be able to write more than the 64k default buffer size to child process stdout without deadlocking the Ruby parent process" do
+      # we add some additional bytes so it's not always on a 1KB boundary and forces pipe reading in different lengths, not always 1K chunks
       result = manager.execute(<<-CODE
-$bytes_in_k = (1024 * 64) + 1
+$bytes_in_k = (1024 * 64) + 11
 [Text.Encoding]::UTF8.GetString((New-Object Byte[] ($bytes_in_k))) | #{output_cmdlet(ps_command, ps_args)}
         CODE
         )
 
       expect(result[:errormessage]).to eq(nil)
       expect(result[:exitcode]).to eq(0)
-      expected = "\x0" * (1024 * 64 + 1) + line_end
+      expected = "\x0" * (1024 * 64 + 11) + line_end
       expect(result[:stdout].length).to eq(expected.length)
       expect(result[:stdout]).to eq(expected)
     end