Skip to content

Fix Net::ReadTimeout errors in net-pop by handling multi-byte terminators getting split up in readuntil #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
astapinski wants to merge 4 commits into
base: master
Choose a base branch
from
+24 −1
Open

Fix Net::ReadTimeout errors in net-pop by handling multi-byte terminators getting split up in readuntil #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eac2c7e
Set offset for terminator searching to use the terminator bytesize
Apr 21, 2024
cc58835
Tweak the readuntil offset logic
Apr 24, 2024
5d63ff1
Add a test for multi-byte terminators in each_message_chunk and readu…
May 13, 2024
5f887c1
Replace each_message_chunk calculation with BUFSIZE
May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/net/protocol.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,10 @@ def readuntil(terminator, ignore_eof = false)
offset = @rbuf_offset
begin
until idx = @rbuf.index(terminator, offset)
offset = @rbuf.bytesize
new_offset = @rbuf.bytesize - terminator.bytesize + 1
# Only assign the offset if it will advance.
# Otherwise an empty @rbuf could result in a negative offset.
Copy link
Author

@astapinski astapinski May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix here was to just set the offset in a way that it should only be set far enough ahead that a multiple byte terminator would still be read completely on the next read. For the comment, I'm not honestly sure if an empty buffer could be an issue but I played it safe and checked that new_offset is greater than offset before setting it.

offset = new_offset if new_offset > offset
rbuf_fill
end
return rbuf_consume(idx + terminator.bytesize - @rbuf_offset)
Expand Down
20 changes: 20 additions & 0 deletions test/net/protocol/test_protocol.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,26 @@ def test_each_crlf_line
end
end

def test_each_message_chunk_boundary
Copy link
Author

@astapinski astapinski May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will fail if one uses the old logic for readuntil as two chunks will be read together.

assert_output("", "") do
# Create this first chunk where the chunk will end with the
# line terminator \r\n only being partially read up to and including \r.
chunk_1 = "#{"1" * (Net::BufferedIO::BUFSIZE - 1)}\r\n".b
chunk_2 = "Second line\r\n".b
chunk_3 = "Third line\r\n".b
test_string = "#{chunk_1}#{chunk_2}#{chunk_3}".b
sio = StringIO.new("#{test_string}.\r\n".b)
io = Net::InternetMessageIO.new(sio)
expected_chunks = []
io.each_message_chunk do |chunk|
expected_chunks << chunk
end
assert_equal chunk_1, expected_chunks[0]
assert_equal chunk_2, expected_chunks[1]
assert_equal chunk_3, expected_chunks[2]
end
end

def create_mockio(capacity: 100, max: nil)
mockio = Object.new
mockio.instance_variable_set(:@str, +'')
Expand Down