From c150772edb65f727c9be0a82ec71c4b084c9d949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 23 Nov 2018 16:46:33 +0100 Subject: [PATCH] Fix deadlock on ChunkedIO --- .../unreleased/fix-deadlock-chunked-io.yml | 5 ++ lib/gitlab/ci/trace/chunked_io.rb | 29 +++++---- lib/gitlab/http_io.rb | 8 +-- spec/lib/gitlab/ci/trace/chunked_io_spec.rb | 63 +++++++++++++++++++ 4 files changed, 88 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/fix-deadlock-chunked-io.yml diff --git a/changelogs/unreleased/fix-deadlock-chunked-io.yml b/changelogs/unreleased/fix-deadlock-chunked-io.yml new file mode 100644 index 00000000000..def7a59e86e --- /dev/null +++ b/changelogs/unreleased/fix-deadlock-chunked-io.yml @@ -0,0 +1,5 @@ +--- +title: Fix deadlock on ChunkedIO +merge_request: +author: +type: fixed diff --git a/lib/gitlab/ci/trace/chunked_io.rb b/lib/gitlab/ci/trace/chunked_io.rb index e9b3199d56e..8c6fd56493f 100644 --- a/lib/gitlab/ci/trace/chunked_io.rb +++ b/lib/gitlab/ci/trace/chunked_io.rb @@ -13,7 +13,7 @@ module Gitlab attr_reader :build attr_reader :tell, :size - attr_reader :chunk, :chunk_range + attr_reader :chunk_data, :chunk_range alias_method :pos, :tell @@ -75,14 +75,14 @@ module Gitlab until length <= 0 || eof? data = chunk_slice_from_offset - break if data.empty? + raise FailedToGetChunkError if data.empty? chunk_bytes = [CHUNK_SIZE - chunk_offset, length].min - chunk_data = data.byteslice(0, chunk_bytes) + chunk_data_slice = data.byteslice(0, chunk_bytes) - out << chunk_data - @tell += chunk_data.bytesize - length -= chunk_data.bytesize + out << chunk_data_slice + @tell += chunk_data_slice.bytesize + length -= chunk_data_slice.bytesize end out = out.join @@ -100,11 +100,14 @@ module Gitlab until eof? data = chunk_slice_from_offset + raise FailedToGetChunkError if data.empty? + new_line = data.index("\n") if !new_line.nil? - out << data[0..new_line] - @tell += new_line + 1 + raw_data = data[0..new_line] + out << raw_data + @tell += raw_data.bytesize break else out << data @@ -121,13 +124,13 @@ module Gitlab while tell < start_pos + data.bytesize # get slice from current offset till the end where it falls into chunk chunk_bytes = CHUNK_SIZE - chunk_offset - chunk_data = data.byteslice(tell - start_pos, chunk_bytes) + data_slice = data.byteslice(tell - start_pos, chunk_bytes) # append data to chunk, overwriting from that point - ensure_chunk.append(chunk_data, chunk_offset) + ensure_chunk.append(data_slice, chunk_offset) # move offsets within buffer - @tell += chunk_data.bytesize + @tell += data_slice.bytesize @size = [size, tell].max end @@ -183,12 +186,12 @@ module Gitlab current_chunk.tap do |chunk| raise FailedToGetChunkError unless chunk - @chunk = chunk.data + @chunk_data = chunk.data @chunk_range = chunk.range end end - @chunk[chunk_offset..CHUNK_SIZE] + @chunk_data.byteslice(chunk_offset, CHUNK_SIZE) end def chunk_offset diff --git a/lib/gitlab/http_io.rb b/lib/gitlab/http_io.rb index e768b8adb12..6a9fb85b054 100644 --- a/lib/gitlab/http_io.rb +++ b/lib/gitlab/http_io.rb @@ -85,11 +85,11 @@ module Gitlab break if data.empty? chunk_bytes = [BUFFER_SIZE - chunk_offset, length].min - chunk_data = data.byteslice(0, chunk_bytes) + data_slice = data.byteslice(0, chunk_bytes) - out << chunk_data - @tell += chunk_data.bytesize - length -= chunk_data.bytesize + out << data_slice + @tell += data_slice.bytesize + length -= data_slice.bytesize end out = out.join diff --git a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb b/spec/lib/gitlab/ci/trace/chunked_io_spec.rb index 6259b952add..546a9e7d0cc 100644 --- a/spec/lib/gitlab/ci/trace/chunked_io_spec.rb +++ b/spec/lib/gitlab/ci/trace/chunked_io_spec.rb @@ -116,6 +116,19 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do chunked_io.each_line { |line| } end end + + context 'when buffer consist of many empty lines' do + let(:sample_trace_raw) { Array.new(10, " ").join("\n") } + + before do + build.trace.set(sample_trace_raw) + end + + it 'yields lines' do + expect { |b| chunked_io.each_line(&b) } + .to yield_successive_args(*string_io.each_line.to_a) + end + end end context "#read" do @@ -143,6 +156,22 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do end end + context 'when chunk is missing data' do + let(:length) { nil } + + before do + stub_buffer_size(1024) + build.trace.set(sample_trace_raw) + + # make second chunk to not have data + build.trace_chunks.second.append('', 0) + end + + it 'raises an error' do + expect { subject }.to raise_error described_class::FailedToGetChunkError + end + end + context 'when read only first 100 bytes' do let(:length) { 100 } @@ -266,6 +295,40 @@ describe Gitlab::Ci::Trace::ChunkedIO, :clean_gitlab_redis_cache do expect(chunked_io.readline).to eq(string_io.readline) end end + + context 'when chunk is missing data' do + let(:length) { nil } + + before do + build.trace.set(sample_trace_raw) + + # make first chunk to have invalid data + build.trace_chunks.first.append('data', 0) + end + + it 'raises an error' do + expect { subject }.to raise_error described_class::FailedToGetChunkError + end + end + + context 'when utf-8 is being used' do + let(:sample_trace_raw) { sample_trace_raw_utf8.force_encoding(Encoding::BINARY) } + let(:sample_trace_raw_utf8) { "😺\n😺\n😺\n😺" } + + before do + stub_buffer_size(3) # the utf-8 character has 4 bytes + + build.trace.set(sample_trace_raw_utf8) + end + + it 'has known length' do + expect(sample_trace_raw_utf8.bytesize).to eq(4 * 4 + 3 * 1) + expect(sample_trace_raw.bytesize).to eq(4 * 4 + 3 * 1) + expect(chunked_io.size).to eq(4 * 4 + 3 * 1) + end + + it_behaves_like 'all line matching' + end end context "#write" do