JRuby

[1.9] IO.ungetc needs to accept multibyte char

Details

  • Number of attachments :
    1

Description

This is a tricky one. IO#ungetc needs to accept a String and unget the first character. Currently, we only support ungetting a byte, so this requires changing ChannelStream too.

This missing feature leads to at least two rubyspec failures:

4)
IO#readpartial reads after ungetc with data in the buffer ERROR
TypeError: can't convert String into Integer
org/jruby/RubyIO.java:2389:in `ungetc19'
/Users/headius/projects/jruby/spec/ruby/core/io/readpartial_spec.rb:35:in `(root)'
org/jruby/RubyBasicObject.java:1697:in `instance_eval19'
org/jruby/RubyEnumerable.java:1260:in `all_p'
org/jruby/RubyArray.java:1676:in `each'
/Users/headius/projects/jruby/spec/ruby/core/io/readpartial_spec.rb:4:in `(root)'
org/jruby/RubyKernel.java:1075:in `load19'
/Users/headius/projects/jruby/spec/ruby/core/io/readpartial_spec.rb:56:in `files'
org/jruby/RubyBasicObject.java:1697:in `instance_eval19'
org/jruby/RubyArray.java:1676:in `each'

5)
IO#readpartial reads after ungetc without data in the buffer ERROR
TypeError: can't convert String into Integer
org/jruby/RubyIO.java:2389:in `ungetc19'
/Users/headius/projects/jruby/spec/ruby/core/io/readpartial_spec.rb:43:in `(root)'
org/jruby/RubyBasicObject.java:1697:in `instance_eval19'
org/jruby/RubyEnumerable.java:1260:in `all_p'
org/jruby/RubyArray.java:1676:in `each'
/Users/headius/projects/jruby/spec/ruby/core/io/readpartial_spec.rb:4:in `(root)'
org/jruby/RubyKernel.java:1075:in `load19'
/Users/headius/projects/jruby/spec/ruby/core/io/readpartial_spec.rb:56:in `files'
org/jruby/RubyBasicObject.java:1697:in `instance_eval19'
org/jruby/RubyArray.java:1676:in `each'

Issue Links

relates to

Bug - A problem which impairs or prevents the functions of the product. JRUBY-5465 [1.9] ChannelStream doesn't handle multibyte characters from ungetc properly

  • Critical - Crashes, loss of data, severe memory leak.
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
David Calavera added a comment - - edited

This patch solves the problem but I'm not sure if it breaks any other cases:

diff --git a/src/org/jruby/RubyIO.java b/src/org/jruby/RubyIO.java
index 808f670..bc42c17 100644
--- a/src/org/jruby/RubyIO.java
+++ b/src/org/jruby/RubyIO.java
@@ -2398,6 +2398,11 @@ public class RubyIO extends RubyObject {
             return getRuntime().getNil();
         }
 
+        if (number instanceof RubyString) {
+            RubyString str = (RubyString) number;
+            number = getRuntime().newFixnum(str.getByteList().get(0));
+        }
+
         return ungetcCommon(number, myOpenFile);
     }

If you agree I can push it with a regresion test.

Show
David Calavera added a comment - - edited This patch solves the problem but I'm not sure if it breaks any other cases: diff --git a/src/org/jruby/RubyIO.java b/src/org/jruby/RubyIO.java index 808f670..bc42c17 100644 --- a/src/org/jruby/RubyIO.java +++ b/src/org/jruby/RubyIO.java @@ -2398,6 +2398,11 @@ public class RubyIO extends RubyObject { return getRuntime().getNil(); } + if (number instanceof RubyString) { + RubyString str = (RubyString) number; + number = getRuntime().newFixnum(str.getByteList().get(0)); + } + return ungetcCommon(number, myOpenFile); } If you agree I can push it with a regresion test.
Hide
Permalink
Charles Oliver Nutter added a comment -

This might not be a bad half-way patch, but I think ungetc needs to actually unread the full character and save it. This would mean most likely saving off a 32-bit codepoint for an ungotten character rather than saving off a byte value as in 1.8 mode.

Show
Charles Oliver Nutter added a comment - This might not be a bad half-way patch, but I think ungetc needs to actually unread the full character and save it. This would mean most likely saving off a 32-bit codepoint for an ungotten character rather than saving off a byte value as in 1.8 mode.
Hide
Permalink
David Calavera added a comment -

I include a patch that fix the problem with IO#ungetc, but as Charlie said we need to modify ChannelStream because it doesn't behave properly with characters stored in multiple bytes after calling that method.

If we run the test cases that I include with Ruby 1.9.2 there is one failure because of that.

Please, apply it if you think it's worth having it for RC2.

Show
David Calavera added a comment - I include a patch that fix the problem with IO#ungetc, but as Charlie said we need to modify ChannelStream because it doesn't behave properly with characters stored in multiple bytes after calling that method. If we run the test cases that I include with Ruby 1.9.2 there is one failure because of that. Please, apply it if you think it's worth having it for RC2.
Hide
Permalink
Charles Oliver Nutter added a comment -

David: I noticed you went ahead with a fix that actually uses codepoint, so that's cool. If you feel like doing a bit extra, it would be nice to get those tests you wrote re-submitted as RubySpecs. Then others might be more likely to add additional cases, improving our test coverage

And if we're 100% now, go ahead and resolve this bug

Show
Charles Oliver Nutter added a comment - David: I noticed you went ahead with a fix that actually uses codepoint, so that's cool. If you feel like doing a bit extra, it would be nice to get those tests you wrote re-submitted as RubySpecs. Then others might be more likely to add additional cases, improving our test coverage And if we're 100% now, go ahead and resolve this bug
Hide
Permalink
David Calavera added a comment -

I reverted the commit because it's not fully compatible yet. ChannelStream doesn't work properly with multibyte characters and when we read the buffer again after the ungetc call the result is not right.

Yep, I have to move the tests to Rubyspec too.

Show
David Calavera added a comment - I reverted the commit because it's not fully compatible yet. ChannelStream doesn't work properly with multibyte characters and when we read the buffer again after the ungetc call the result is not right. Yep, I have to move the tests to Rubyspec too.

People

  • Assignee:
    Unassigned
    Reporter:
    Charles Oliver Nutter
Vote (0)
Watch (0)

Dates

  • Created:
    Updated: