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.

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

CSV.parse tries to close a file #4298

Closed
rovf opened this issue Nov 15, 2016 · 16 comments
Closed

CSV.parse tries to close a file #4298

rovf opened this issue Nov 15, 2016 · 16 comments

Comments

@rovf
Copy link

rovf commented Nov 15, 2016

Code to execute:

require 'csv'; CSV.parse nil

Environment

jruby 1.7.25 (1.9.3p551) 2016-04-13 867cb81 on Java HotSpot(TM) 64-Bit Server VM 1.7.0_79-b15 +jit [Windows 7-amd64]

CYGWIN_NT-6.1 CMTCL033974 2.4.1(0.293/5/3) 2016-01-24 11:26 x86_64 Cygwin

Expected Behavior

An exception of type ArgumentError, saying that the argument to parse is incorrect.

Actual Behavior

An exception of type NoMethodError :

irb(main):031:0> CSV.parse nil
NoMethodError: undefined method close' for nil:NilClass from /C:/cygwin64/home/fisrona/gitwrk/vp5/repository/maven2/org/jruby/jruby-complete/1.7.25/jruby-complete-1.7.25.jar!/META-INF/jruby.home/lib/ruby/1.9/csv.rb:1381:in parse'
from (irb):31:in evaluate' from org/jruby/RubyKernel.java:1079:in eval'
from org/jruby/RubyKernel.java:1479:in loop' from org/jruby/RubyKernel.java:1242:in catch'
from org/jruby/RubyKernel.java:1242:in catch' from C:/cygwin64/home/FISRONA/gitwrk/vp5/bin/jirb_swing_ex:64:in (root)'
irb(main):032:0>

@olleolleolle
Copy link
Member

olleolleolle commented Nov 22, 2016

Update: Using it from a jruby-complete-9.1.6.0.jar works the same:

 ~ java -Xmx500m -Xss1024k -jar jruby-complete-9.1.6.0.jar -e 'require "csv"; CSV.parse nil'
ArgumentError: Cannot parse nil as CSV
  initialize at uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/csv.rb:1510
       parse at uri:classloader:/META-INF/jruby.home/lib/ruby/stdlib/csv.rb:1304
      <main> at -e:1

Update: Shorter jruby-complete-1.7.26.jar reproduce on OS X.

 ~ java -Xmx500m -Xss1024k -jar jruby-complete-1.7.26.jar -e 'require "csv"; CSV.parse nil'
NoMethodError: undefined method `close' for nil:NilClass
   close at /Users/olle/jruby-complete-1.7.26.jar!/META-INF/jruby.home/lib/ruby/1.9/forwardable.rb:201
   parse at /Users/olle/jruby-complete-1.7.26.jar!/META-INF/jruby.home/lib/ruby/1.9/csv.rb:1381
  (root) at -e:1

@rovf
Copy link
Author

rovf commented Nov 22, 2016

Yes, that's exactly what I meant. The response from MRI Ruby makes sense. The one produced by JRuby does not.

@olleolleolle
Copy link
Member

olleolleolle commented Nov 22, 2016

(Your reproduce steps were perfect - I added the exact wording of the ArgumentError + the stacktrace from MRI - only for comparison.)

Can you reproduce the error using the 1.7.26, which is latest stable release?

@rovf
Copy link
Author

rovf commented Nov 22, 2016

Yes, same behaviour as 1.7.25.

@olleolleolle
Copy link
Member

olleolleolle commented Nov 22, 2016

Actually, this is at feature parity with Ruby 1.9.3.

$ rvm 1.9.3 do ruby -e 'require "csv"; CSV.parse nil'
Ruby ruby-1.9.3-p551 is not installed.
$ rvm install 1.9.3

... installing.

$ rvm 1.9.3 do ruby -e 'require "csv"; CSV.parse nil'
/Users/olle/.rvm/rubies/ruby-1.9.3-p551/lib/ruby/1.9.1/csv.rb:1381:in `ensure in parse': undefined method `close' for nil:NilClass (NoMethodError)
	from /Users/olle/.rvm/rubies/ruby-1.9.3-p551/lib/ruby/1.9.1/csv.rb:1381:in `parse'
	from -e:1:in `<main>'

@rovf
Copy link
Author

rovf commented Nov 22, 2016

Hmmmm, interesting. I'm not sure I would call it a feature, but rather something which was not noticed earlier, and was fixed in 2.x.

Wouldn't it make sense to backport the fix? Actually, to a reader, it is not obvious with this message that the argument to parse was nil. The first impression is that it must be a bug in Ruby. On the other hand, the exception thrown by Ruby 2.x clearly points out that the argument to close was incorrect.

@olleolleolle
Copy link
Member

@rovf JRuby 1.7 is EOL-marked for the close of this year. #4112

@rovf
Copy link
Author

rovf commented Nov 22, 2016

This is a pity, as it is impossible for me to upgrade, even though I would love to upgrade (and, since it is rare that a certain condition applies to a single person on the world, I could imagine that other users are affected too).

In any case, there are still some weeks until then, and with some luck, a backport is not so difficult? ;-)

@enebo
Copy link
Member

enebo commented Nov 22, 2016

@rovf part of the issue is that there might be people who have 1.9 code which depends on catching NoMethodError. If we change the exception raised it will break their code. Is that unlikely? I guess I don't know but this is one reason we try and stay as close to MRI as we can.

@rovf
Copy link
Author

rovf commented Nov 22, 2016

IMHO:

First, it is unlikely that someone specifically catches NoMethodError in this case, because it is not really logical. More likely, RuntimeError or Exception will be caught.

Second, even if someone does, the code would be broken once he upgrades to JRuby 2000.

Finally, if someone still uses JRuby 1.7 now near the end of its lifetime, and upgrades a last time, he will for sure read about all the changes, simply because in this state, the list of changes is likely short.

So, no, I'm willing to bet a good bottle of German wine, that no code will break just because of this change within the 1.7 line.

@enebo
Copy link
Member

enebo commented Nov 22, 2016

@rovf generally it is our policy to be bug for bug compat with MRI version we are supporting because we cannot know how people will react to those bugs in MRI. I did a search for this and found an example of catching NoMethodError in the first page of results in github:

https://github.com/apartmentlist/bulk-processor/blob/980acf26b0e64a127fbf6330ea1580fa281dbeda/lib/bulk_processor/validated_csv.rb#L57

You said this is not logical and it would not be something you would add until you hit the weird error you encountered. Then you might add the most specific error possible in case you later discover another weird error in CSV vs generic rescue which would wallpaper over it.

Another point would be that when people do update to JRuby 9k they probably expect some transitioning from 1.9 to 2.x since it is a major update. At least in 2.x we get a sensible error so they should be able to easily correct the problem.

@headius
Copy link
Member

headius commented Nov 22, 2016

@rovf Why is it impossible for you to upgrade to JRuby 9k? Please add issues for anything stopping you and reference them at our 1.7 EOL issue (#4112).

@rovf
Copy link
Author

rovf commented Nov 23, 2016

@enebo: I understand your argument, that you want to be bug-compatible. Just want to notice that it is not backed up by the example you gave. If you look at the code which you found on Github, you see that NoMethodError is caught only for the BAD_HEADERS error. In particular, a nil argument to parse would propagate upwards, because the exception would be re-raised in the 'else' part of the statement.

@rovf
Copy link
Author

rovf commented Nov 23, 2016

@headius: JRuby 9k requires Java 1.7. In many (large) enterprises - for example a really huge company I'm working for -, the requirement for enterprise solution is still Java 1.6.

This might sound surprising, but has a simple reason: For security reason, any new software needs to get internal certification. The problem in this respect is not so much for the JVM itself, but from the application server and other applications running on it. If the application server and/or some of the business software happened to be written, when JVM 1.6 is the target platform, the requirement is to stick with 1.6, until everything is certified with 1.7. In big companies, this is a tedious process, and usually not one which is considered "urgent" (because new developments can, technically, be done in Java 1.6 as well).

Of course, this state won't keep for ever, but I still expect JVM 1.6 environments to be around for another couple of years.

@enebo
Copy link
Member

enebo commented Nov 23, 2016

@rovf I did find a more ambiguous error (it would catch that error but it wraps more code than just that):

https://github.com/slctd/tigr-crm/blob/7009bd4420dbefb3669310420f4adea54c8e9d09/lib/importer.rb#L63

In any case, you do understand why I made the point I did so I will not belabor the point any longer. The longer I support this project the more I think someone is doing something somewhere :)

@kares
Copy link
Member

kares commented Dec 1, 2016

I also have dealt with wanting 'newer' std-libraries in JRuby 1.7 ... its usually possible since - its just Ruby.
never occurred to me to blame it as a JRuby 1.7 bug ... instead what I generally did:

  • review a newer version to not use new features (its usually just fine with pieces such as csv.rb)
  • move the updated .rb files into a vendor directory
  • the usual $LOAD_PATH magic to make sure the updated files are being loaded

@rovf also I love "old" enterprise deployments - feel free to get in touch when help is needed :)
... also to be fair for security reasons no one should be on Java 6 (except the paid long-term version) but you still do get compatible jruby-openssl releases thus you guys might be just fine on JRuby 1.7 for a while.

@kares kares closed this as completed Dec 1, 2016
@kares kares added this to the Won't Fix milestone Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants