From 243c56a9d56426c2d968c47ea67f206b2fce4471 Mon Sep 17 00:00:00 2001 From: Charles Oliver Nutter Date: Fri, 20 Oct 2023 20:30:50 -0500 Subject: [PATCH] Update test_io_buffer and make fixes * This includes two PRs to improve behavior and testing: * https://github.com/ruby/ruby/pull/8728 * https://github.com/ruby/ruby/pull/8729 --- .../src/main/java/org/jruby/RubyIOBuffer.java | 120 ++++++++++++++---- test/mri/ruby/test_io_buffer.rb | 100 ++++++++++++--- 2 files changed, 177 insertions(+), 43 deletions(-) diff --git a/core/src/main/java/org/jruby/RubyIOBuffer.java b/core/src/main/java/org/jruby/RubyIOBuffer.java index 93cddc756f4..092e0333207 100644 --- a/core/src/main/java/org/jruby/RubyIOBuffer.java +++ b/core/src/main/java/org/jruby/RubyIOBuffer.java @@ -78,6 +78,13 @@ public static RubyIOBuffer newBuffer(Ruby runtime, int size, int flags) { return new RubyIOBuffer(runtime, runtime.getIOBuffer(), newBufferBase(runtime, size, flags), size, flags); } + public static RubyIOBuffer newBuffer(ThreadContext context, RubyString string, int flags) { + ByteList bytes = string.getByteList(); + int size = bytes.realSize(); + + return newBuffer(context.runtime, ByteBuffer.wrap(bytes.unsafeBytes(), bytes.begin(), size), size, flags); + } + public RubyIOBuffer(Ruby runtime, RubyClass metaClass) { super(runtime, metaClass); } @@ -107,11 +114,7 @@ public static IRubyObject rbFor(ThreadContext context, IRubyObject self, IRubyOb } } - ByteList bytes = string.getByteList(); - int size = bytes.realSize(); - ByteBuffer wrap = ByteBuffer.wrap(bytes.unsafeBytes(), bytes.begin(), size); - - RubyIOBuffer buffer = newBuffer(context.runtime, wrap, size, flags); + RubyIOBuffer buffer = newBuffer(context, string, flags); if (block.isGiven()) { return block.yieldSpecific(context, buffer); @@ -120,6 +123,23 @@ public static IRubyObject rbFor(ThreadContext context, IRubyObject self, IRubyOb return buffer; } + @JRubyMethod(meta = true) + public static IRubyObject string(ThreadContext context, IRubyObject self, IRubyObject _length, Block block) { + Ruby runtime = context.runtime; + + int size = _length.convertToInteger().getIntValue(); + if (size < 0) throw runtime.newArgumentError("negative string size (or size too big)"); + RubyString string = RubyString.newString(runtime, new byte[size]); + ByteList bytes = string.getByteList(); + ByteBuffer wrap = ByteBuffer.wrap(bytes.unsafeBytes(), bytes.begin(), size); + + RubyIOBuffer buffer = newBuffer(context.runtime, wrap, size, 0); + + block.yieldSpecific(context, buffer); + + return string; + } + @JRubyMethod(name = "map", meta = true) public static IRubyObject map(ThreadContext context, IRubyObject self, IRubyObject _file) { RubyFile file = checkFile(context, _file); @@ -701,7 +721,7 @@ public IRubyObject slice(ThreadContext context, int offset, int length) { // MRI: io_buffer_validate_range private void validateRange(ThreadContext context, int offset, int length) { if (offset + length > size) { - throw context.runtime.newArgumentError("Specified offset+length exceeds data size!"); + throw context.runtime.newArgumentError("Specified offset+length is bigger than the buffer size!"); } } @@ -1427,6 +1447,7 @@ public IRubyObject copy(ThreadContext context, RubyIOBuffer source, int offset, return RubyFixnum.newFixnum(context.runtime, length); } + // MRI: io_buffer_copy_from public IRubyObject copy(ThreadContext context, RubyString source, int offset, int length, int sourceOffset) { if (sourceOffset > length) { throw context.runtime.newArgumentError("The given source offset is bigger than the source itself!"); @@ -1471,23 +1492,23 @@ public IRubyObject get_string(ThreadContext context) { @JRubyMethod(name = "get_string") public IRubyObject get_string(ThreadContext context, IRubyObject _offset) { - int offset = RubyNumeric.num2int(_offset); + int offset = extractOffset(context, _offset); return getString(context, offset, size, ASCIIEncoding.INSTANCE); } @JRubyMethod(name = "get_string") public IRubyObject get_string(ThreadContext context, IRubyObject _offset, IRubyObject _length) { - int offset = RubyNumeric.num2int(_offset); - int length = RubyNumeric.num2int(_length); + int offset = extractOffset(context, _offset); + int length = extractLength(context, _length, offset); return getString(context, offset, length, ASCIIEncoding.INSTANCE); } @JRubyMethod(name = "get_string") public IRubyObject get_string(ThreadContext context, IRubyObject _offset, IRubyObject _length, IRubyObject _encoding) { - int offset = RubyNumeric.num2int(_offset); - int length = RubyNumeric.num2int(_length); + int offset = extractOffset(context, _offset); + int length = extractLength(context, _length, offset); Encoding encoding = context.runtime.getEncodingService().getEncodingFromObject(_encoding); return getString(context, offset, length, encoding); @@ -1520,7 +1541,7 @@ public IRubyObject set_string(ThreadContext context, IRubyObject _string) { @JRubyMethod(name = "set_string") public IRubyObject set_string(ThreadContext context, IRubyObject _string, IRubyObject _offset) { RubyString string = _string.convertToString(); - int offset = RubyNumeric.num2int(_offset); + int offset = extractOffset(context, _offset); return copy(context, string, offset, string.size(), 0); } @@ -1528,8 +1549,8 @@ public IRubyObject set_string(ThreadContext context, IRubyObject _string, IRubyO @JRubyMethod(name = "set_string") public IRubyObject set_string(ThreadContext context, IRubyObject _string, IRubyObject _offset, IRubyObject _length) { RubyString string = _string.convertToString(); - int offset = RubyNumeric.num2int(_offset); - int length = RubyNumeric.num2int(_length); + int offset = extractOffset(context, _offset); + int length = extractLength(context, _length, offset); return copy(context, string, offset, length, 0); } @@ -1739,28 +1760,59 @@ private static void bufferNotInPlace(ByteBuffer a, int aSize) { } @JRubyMethod(name = "read") - public IRubyObject read(ThreadContext context, IRubyObject io, IRubyObject length) { + public IRubyObject read(ThreadContext context, IRubyObject io) { IRubyObject scheduler = context.getFiberCurrentThread().getSchedulerCurrent(); - RubyInteger lengthInteger = length.convertToInteger(); if (!scheduler.isNil()) { - IRubyObject result = FiberScheduler.ioRead(context, scheduler, io, this, lengthInteger, RubyFixnum.zero(context.runtime)); + Ruby runtime = context.runtime; + IRubyObject result = FiberScheduler.ioRead(context, scheduler, io, this, RubyFixnum.newFixnum(runtime, size), RubyFixnum.zero(runtime)); if (result != UNDEF) { return result; } } + return read(context, io, size, 0); + } + + @JRubyMethod(name = "read") + public IRubyObject read(ThreadContext context, IRubyObject io, IRubyObject _length) { + if (_length.isNil()) { + return read(context, io); + } + + IRubyObject scheduler = context.getFiberCurrentThread().getSchedulerCurrent(); + RubyInteger lengthInteger = _length.convertToInteger(); + + if (!scheduler.isNil()) { + IRubyObject result = FiberScheduler.ioRead(context, scheduler, io, this, lengthInteger, RubyFixnum.zero(context.runtime)); + + if (result != null) { + return result; + } + } + return read(context, io, lengthInteger.getIntValue(), 0); } @JRubyMethod(name = "read") - public IRubyObject read(ThreadContext context, IRubyObject io, IRubyObject length, IRubyObject offset) { + public IRubyObject read(ThreadContext context, IRubyObject io, IRubyObject _length, IRubyObject _offset) { IRubyObject scheduler = context.getFiberCurrentThread().getSchedulerCurrent(); - RubyInteger lengthInteger = length.convertToInteger(); - RubyInteger offsetInteger = offset.convertToInteger(); + RubyInteger offsetInteger = _offset.convertToInteger(); + int offset = offsetInteger.getIntValue(); + + int length; + RubyInteger lengthInteger; + if (_length.isNil()) { + length = size - offset; + lengthInteger = null; + } else { + lengthInteger = _length.convertToInteger(); + length = lengthInteger.getIntValue(); + } if (!scheduler.isNil()) { + if (lengthInteger == null) lengthInteger = RubyFixnum.newFixnum(context.runtime, length); IRubyObject result = FiberScheduler.ioRead(context, scheduler, io, this, lengthInteger, offsetInteger); if (result != UNDEF) { @@ -1768,7 +1820,7 @@ public IRubyObject read(ThreadContext context, IRubyObject io, IRubyObject lengt } } - return read(context, io, lengthInteger.getIntValue(), offsetInteger.getIntValue()); + return read(context, io, length, offset); } public IRubyObject read(ThreadContext context, IRubyObject io, int length, int offset) { @@ -1963,6 +2015,22 @@ private static int extractSize(ThreadContext context, IRubyObject _size) { return RubyNumeric.num2int(_size); } + @JRubyMethod(name = "write") + public IRubyObject write(ThreadContext context, IRubyObject io) { + IRubyObject scheduler = context.getFiberCurrentThread().getSchedulerCurrent(); + + if (!scheduler.isNil()) { + Ruby runtime = context.runtime; + IRubyObject result = FiberScheduler.ioWrite(context, scheduler, io, this, RubyFixnum.newFixnum(runtime, size), RubyFixnum.zero(runtime)); + + if (result != null) { + return result; + } + } + + return write(context, io, size, 0); + } + @JRubyMethod(name = "write") public IRubyObject write(ThreadContext context, IRubyObject io, IRubyObject length) { IRubyObject scheduler = context.getFiberCurrentThread().getSchedulerCurrent(); @@ -1971,7 +2039,7 @@ public IRubyObject write(ThreadContext context, IRubyObject io, IRubyObject leng if (!scheduler.isNil()) { IRubyObject result = FiberScheduler.ioWrite(context, scheduler, io, this, lengthInteger, RubyFixnum.zero(context.runtime)); - if (result != UNDEF) { + if (result != null) { return result; } } @@ -1988,7 +2056,7 @@ public IRubyObject write(ThreadContext context, IRubyObject io, IRubyObject leng if (!scheduler.isNil()) { IRubyObject result = FiberScheduler.ioWrite(context, scheduler, io, this, lengthInteger, offsetInteger); - if (result != UNDEF) { + if (result != null) { return result; } } @@ -2029,7 +2097,7 @@ public IRubyObject pwrite(ThreadContext context, IRubyObject io, IRubyObject _fr Ruby runtime = context.runtime; IRubyObject result = FiberScheduler.ioPWrite(context, scheduler, io, this, fromInteger, RubyFixnum.newFixnum(runtime, length), RubyFixnum.zero(runtime)); - if (result != UNDEF) { + if (result != null) { return result; } } @@ -2048,7 +2116,7 @@ public IRubyObject pwrite(ThreadContext context, IRubyObject io, IRubyObject _fr if (!scheduler.isNil()) { IRubyObject result = FiberScheduler.ioPWrite(context, scheduler, io, this, fromInteger, lengthInteger, RubyFixnum.zero(context.runtime)); - if (result != UNDEF) { + if (result != null) { return result; } } @@ -2086,7 +2154,7 @@ public IRubyObject pwrite(ThreadContext context, IRubyObject io, IRubyObject _fr if (!scheduler.isNil()) { IRubyObject result = FiberScheduler.ioPWrite(context, scheduler, io, this, fromInteger, lengthInteger, offsetInteger); - if (result != UNDEF) { + if (result != null) { return result; } } diff --git a/test/mri/ruby/test_io_buffer.rb b/test/mri/ruby/test_io_buffer.rb index 1b2160ecc74..dea8388d7dc 100644 --- a/test/mri/ruby/test_io_buffer.rb +++ b/test/mri/ruby/test_io_buffer.rb @@ -80,7 +80,7 @@ def test_file_mapped end def test_file_mapped_invalid - assert_raise NoMethodError do + assert_raise TypeError do IO::Buffer.map("foobar") end end @@ -102,11 +102,6 @@ def test_string_mapped_mutable IO::Buffer.for(string) do |buffer| refute buffer.readonly? - # Cannot modify string as it's locked by the buffer: - assert_raise RuntimeError do - string[0] = "h" - end - buffer.set_value(:U8, 0, "h".ord) # Buffer releases it's ownership of the string: @@ -116,6 +111,16 @@ def test_string_mapped_mutable end end + def test_string_mapped_buffer_locked + string = "Hello World" + IO::Buffer.for(string) do |buffer| + # Cannot modify string as it's locked by the buffer: + assert_raise RuntimeError do + string[0] = "h" + end + end + end + def test_non_string not_string = Object.new @@ -124,6 +129,20 @@ def test_non_string end end + def test_string + result = IO::Buffer.string(12) do |buffer| + buffer.set_string("Hello World!") + end + + assert_equal "Hello World!", result + end + + def test_string_negative + assert_raise ArgumentError do + IO::Buffer.string(-1) + end + end + def test_resize_mapped buffer = IO::Buffer.new @@ -142,6 +161,24 @@ def test_resize_preserve assert_equal message, buffer.get_string(0, message.bytesize) end + def test_resize_zero_internal + buffer = IO::Buffer.new(1) + + buffer.resize(0) + assert_equal 0, buffer.size + + buffer.resize(1) + assert_equal 1, buffer.size + end + + def test_resize_zero_external + buffer = IO::Buffer.for('1') + + assert_raise IO::Buffer::AccessError do + buffer.resize(0) + end + end + def test_compare_same_size buffer1 = IO::Buffer.new(1) assert_equal buffer1, buffer1 @@ -219,6 +256,18 @@ def test_get_string chunk = buffer.get_string(0, message.bytesize, Encoding::BINARY) assert_equal Encoding::BINARY, chunk.encoding + + assert_raise_with_message(ArgumentError, /bigger than the buffer size/) do + buffer.get_string(0, 129) + end + + assert_raise_with_message(ArgumentError, /bigger than the buffer size/) do + buffer.get_string(129) + end + + assert_raise_with_message(ArgumentError, /Offset can't be negative/) do + buffer.get_string(-1) + end end # We check that values are correctly round tripped. @@ -329,21 +378,38 @@ def test_invalidation input.close end - def test_read - # This is currently a bug in IO:Buffer [#19084] which affects extended - # strings. On 32 bit machines, the example below becomes extended, so - # we omit this test until the bug is fixed. - omit if RUBY_ENGINE == 'ruby' && GC::INTERNAL_CONSTANTS[:SIZE_POOL_COUNT] == 1 + def hello_world_tempfile io = Tempfile.new io.write("Hello World") io.seek(0) - buffer = IO::Buffer.new(128) - buffer.read(io, 5) - - assert_equal "Hello", buffer.get_string(0, 5) + yield io ensure - io.close! if io + io&.close! + end + + def test_read + hello_world_tempfile do |io| + buffer = IO::Buffer.new(128) + buffer.read(io) + assert_equal "Hello", buffer.get_string(0, 5) + end + end + + def test_read_with_with_length + hello_world_tempfile do |io| + buffer = IO::Buffer.new(128) + buffer.read(io, 5) + assert_equal "Hello", buffer.get_string(0, 5) + end + end + + def test_read_with_with_offset + hello_world_tempfile do |io| + buffer = IO::Buffer.new(128) + buffer.read(io, nil, 6) + assert_equal "Hello", buffer.get_string(6, 5) + end end def test_write @@ -351,7 +417,7 @@ def test_write buffer = IO::Buffer.new(128) buffer.set_string("Hello") - buffer.write(io, 5) + buffer.write(io) io.seek(0) assert_equal "Hello", io.read(5)