Skip to content
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

Jump to bottom

Buffer encoding #19

Merged
merged 7 commits into from
Jan 29, 2018
Merged

Buffer encoding #19

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2a86ded
using buffer string in binary encoding, as String.new is utf-8, and y…
HoneyryderChuck Dec 18, 2017
621e18e
use String#bytesize, as #length returns the length size in its encoding
HoneyryderChuck Dec 18, 2017
3d97b83
use String#clear instead of String#replace, as the latter changes the…
HoneyryderChuck Dec 18, 2017
9a1b911
do not use rercarpet for jruby
HoneyryderChuck Dec 18, 2017
3bc3787
jruby accusing something other than ASCII-8bit sometimes (US-ASCII), …
HoneyryderChuck Dec 18, 2017
f08d80c
hash rocketz
HoneyryderChuck Jan 8, 2018
eeff06c
forcing the buffer to binary, as the buffer encoding might change
HoneyryderChuck Jan 29, 2018
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
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ group :test do
end

group :doc do
gem "redcarpet"
gem "redcarpet", platform: :mri
Copy link

Choose a reason for hiding this comment

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

Travis CI is blowing up on this line, because Rubocop for the project is requiring hash rockets. I am guessing that's a personal preference for the project, as I prefer Ruby 1.9+ colon syntax...

gem "yard"
end

Expand Down
8 changes: 5 additions & 3 deletions lib/http/form_data/composite_io.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class CompositeIO
# @param [Array<IO>] ios Array of IO objects
def initialize(ios)
@index = 0
@buffer = String.new
@buffer = "".b
@ios = ios.map do |io|
if io.is_a?(String)
StringIO.new(io)
Expand All @@ -29,14 +29,16 @@ def initialize(ios)
#
# @return [String, nil]
def read(length = nil, outbuf = nil)
outbuf = outbuf.to_s.replace("")
outbuf = outbuf.to_s.clear
Copy link
Member

@tarcieri tarcieri Dec 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Though this is likely a bug in the previous version as well, #to_s may make a new object, at which point the purpose of passing outbuf is defeated.

Perhaps give outbuf a default value (e.g. outbuf = "".b instead of outbuf = nil as the argument definition), and raise if it's not a String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#to_s will not create a new object if the object is a string. But the replace call might change its encoding. That was the reasoning behind it.

Copy link
Member

Choose a reason for hiding this comment

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

Hence "may make a new object". We never want to make a new object for the buffer... that defeats the point.

How about raising if the supplied buffer is not a String type with Encoding::BINARY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never want to make a new object for the buffer... that defeats the point.

ahh, I see what you mean.

How about raising if the supplied buffer is ...

I'm still not sure. The encoding requirement is usually not mandatory, and the buffer passed may be a specialized buffer. Besides "fixing" jruby, I'm not even sure that forcing binary encoding there is the right thing to do.

By the way, what's the reasoning behind allowing length = nil? The length argument is usually required for IO#read and File#read.

Copy link
Member

@tarcieri tarcieri Dec 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

the buffer passed may be a specialized buffer

In what case would it make sense for it to be anything but a String? Ultimately it needs to thunk to a Ruby I/O function which also accepts a preallocated buffer, and those all need to be a String unless JRuby has some fancy behavior I don't know about where it lets you pass a Java ByteBuffer or something.

Besides "fixing" jruby, I'm not even sure that forcing binary encoding there is the right thing to do.

For these types of buffers, I think mandating Encoding::BINARY is the only right option. They should really be a ByteArray/ByteBuffer type, but Ruby doesn't have those, so we need the next best thing, which is a String with Encoding::BINARY.

For other encodings there are all sorts of pathological performance implications, and the contract at this point in the code is to handle arbitrary binary data that may not be whatever-encoding-was-passed-in.

Given this is a rarely used power-user feature, I think it should have some pretty exacting semantics, which are:

  • Avoid all allocations. The whole point of this API is to avoid allocations. This means no #to_s or anything else that might potentially allocate memory
  • String is probably the only argument type that makes sense
  • Encoding::BINARY (i.e. Encoding::ASCII_8BIT) is probably the only sensible encoding for the buffer String. If you think other encodings should be supported, do you really think the answer to the question "Will outbuf always be a valid string under encoding X?" is yes?

By the way, what's the reasoning behind allowing length = nil?

No idea, like the outbuf being nil I think that probably doesn't make sense.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this change, I'll try to answer the questions below.

# buffer in JRuby is sometimes US-ASCII, force to ASCII-8BIT
outbuf.force_encoding(Encoding::BINARY)

while current_io
current_io.read(length, @buffer)
outbuf << @buffer

if length
length -= @buffer.length
length -= @buffer.bytesize
break if length.zero?
end

Expand Down
2 changes: 1 addition & 1 deletion lib/http/form_data/multipart/param.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def self.coerce(data)
private

def header
header = String.new
header = "".b
header << "Content-Disposition: form-data; #{parameters}#{CRLF}"
header << "Content-Type: #{content_type}#{CRLF}" if content_type
header << CRLF
Expand Down