IO.copy_stream with Zlib::GZipWriter broken in 9.1.3.0 and up #4202

Closed
ewr opened this Issue Oct 4, 2016 · 5 comments

Projects

None yet

2 participants

@ewr
ewr commented Oct 4, 2016

In 9.1.3.0 and above, calling IO.copy_stream with a Zlib::GzipWriter breaks due to ArgumentError: wrong number of arguments callingrespond_to?(2 for 1).

I suspect it may have come from 9167f3a.

Reproduction script:

require 'tempfile'
require 'zlib'
require 'pry'

infile = Tempfile.new("copy_stream")
infile << "Test content"

outfile = Tempfile.new("copy_stream")

Zlib::GzipWriter.open(outfile.path) do |gz|
  IO.copy_stream(infile.path, gz)
end

Works on 9.0.4.0 and 9.1.2.0. Fails on 9.1.3.0, 9.1.4.0 and 9.1.5.0.

@headius
Member
headius commented Oct 5, 2016

@ewr Good theory. I don't suppose you've bisected it?

I'll investigate.

@ewr
ewr commented Oct 5, 2016

Nope. Just wandered around looking for respond_to, and noticed that change occurred after the release where things broke.

@headius
Member
headius commented Oct 5, 2016

Oh yeah, this is definitely from that commit. Simple fix coming.

This affects any copy_stream cases that need to coerce the streams. Great find and great investigation, thanks!

@headius
Member
headius commented Oct 5, 2016

For future reference, you can narrow down an exception even more by passing -Xbacktrace.style=full to JRuby, which will include all the hidden Java stack frames. Here's the top of the error, with that flag set:

ArgumentError: wrong number of arguments calling `respond_to?` (2 for 1)
             getStackTrace at java/lang/Thread.java:1552
          getBacktraceData at org/jruby/runtime/backtrace/TraceType.java:246
              getBacktrace at org/jruby/runtime/backtrace/TraceType.java:47
          prepareBacktrace at org/jruby/RubyException.java:236
                  preRaise at org/jruby/exceptions/RaiseException.java:216
                  preRaise at org/jruby/exceptions/RaiseException.java:183
                    <init> at org/jruby/exceptions/RaiseException.java:111
         newRaiseException at org/jruby/Ruby.java:4176
          newArgumentError at org/jruby/Ruby.java:3569
        raiseArgumentError at org/jruby/runtime/Arity.java:277
        raiseArgumentError at org/jruby/internal/runtime/methods/JavaMethod.java:270
                      call at org/jruby/internal/runtime/methods/JavaMethod.java:1072
                      call at org/jruby/internal/runtime/methods/JavaMethod.java:724
                      call at org/jruby/internal/runtime/methods/DynamicMethod.java:205
              cacheAndCall at org/jruby/runtime/callsite/RespondToCallSite.java:161
                      call at org/jruby/runtime/callsite/CachingCallSite.java:195
                respondsTo at org/jruby/runtime/callsite/RespondToCallSite.java:101
               copy_stream at org/jruby/RubyIO.java:4075
                      call at org/jruby/RubyIO$INVOKER$s$0$2$copy_stream.gen:-1
                      call at org/jruby/internal/runtime/methods/JavaMethod.java:724
                      call at org/jruby/internal/runtime/methods/DynamicMethod.java:205
              cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:358
                      call at org/jruby/runtime/callsite/CachingCallSite.java:195
  invokeOther1:copy_stream at blah.rb:11
          block in blah.rb at blah.rb:11
@headius headius added this to the JRuby 9.1.6.0 milestone Oct 6, 2016
@headius headius closed this in 825877f Oct 6, 2016
@headius headius added a commit that referenced this issue Oct 6, 2016
@headius headius Fix regression spec for #4202. 8a4a945
@headius
Member
headius commented Oct 6, 2016

So the actual bug here was that our GzipFile subclasses – GzipReader and GzipWriter – define their own respond_to? implementation, which did not support arity-2 calls (passing in the "include private" argument). I added that path, moved the code up from the child classes into GzipFile, and added a regression spec for this behavior.

MRI does not do this. Instead of having a path method defined that delegates to the underlying IO, they define a new method on every instance as a singleton method. We obviously don't want to do that, so we define path always and delegate respond_to? :path to the underlying IO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment