JRuby

Buffer management defect in RubyIO library: Class ChannelStream

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Critical Critical
  • Resolution: Fixed
  • Affects Version/s: JRuby 1.1RC3
  • Fix Version/s: JRuby 1.1
  • Component/s: Core Classes/Modules
  • Labels:
    None
  • Environment:
    All
  • Patch Submitted:
    Yes
  • Number of attachments :
    0

Description

I was having a problem where larger http POST operations was resulting in truncation of data coming into the Ruby ActionController. I debugged this under Eclipse and discovered the following:

In class ChannelStream there is a method:

private ByteList bufferedRead(int number)

This method reads from an input channel and transfers the data into a ByteList. Since the data can be returned by the channel reader in chunks, there is a loop to read it chunk by chunk, appending it into the ByteList called result. However, the loop control has an apparent fault in it, which results in only the first chunk being read.

The relevant line is:

while (read == BUFSIZE && result.length() != number) { // not complete. try to read more

I haven't analysed this too deeply, other than to observe that in my case, the first call to read from the channel does not return all the bytes available, even though there is room in the buffer. (I don't think the java API guarantees that it will). By fiddling in the debugger, I confirmed that if it went around the loop a second time the remaining bytes were indeed collected.

So, I modified the loop control to remove the first test, since that seemed superfluous to me on superficial examination...

while (result.length() != number)

This appears to have fixed my problem, but could you please confirm that my fix is correct, and that there aren't any other similar bugs in this module?

Activity

Hide
Mike Herrick added a comment -

I experienced similar problem. Milton's fix resolved it. See:
http://archive.jruby.codehaus.org/user/47E5A81A.60100%40csinitiative.com

for details

(the archive seems way behind btw)

Show
Mike Herrick added a comment - I experienced similar problem. Milton's fix resolved it. See: http://archive.jruby.codehaus.org/user/47E5A81A.60100%40csinitiative.com for details (the archive seems way behind btw)
Hide
Charles Oliver Nutter added a comment -

Wow, this is a really good find. I don't see why this wouldn't be correct yet. Marking for JRuby 1.1.1 (possibly could sneak this into 1.1 though).

Show
Charles Oliver Nutter added a comment - Wow, this is a really good find. I don't see why this wouldn't be correct yet. Marking for JRuby 1.1.1 (possibly could sneak this into 1.1 though).
Hide
Mike Herrick added a comment -

I'd suggest it for 1.1. If 2 people have already experienced it in RC3 its bound to be a problem for many people who use JRuby with web apps. I have a release in a week, do you think I am safe enough deploying with Milton's patch and RC3? Or should I stay on RC1? Thanks a lot for all your great work.

Show
Mike Herrick added a comment - I'd suggest it for 1.1. If 2 people have already experienced it in RC3 its bound to be a problem for many people who use JRuby with web apps. I have a release in a week, do you think I am safe enough deploying with Milton's patch and RC3? Or should I stay on RC1? Thanks a lot for all your great work.
Hide
Mike Herrick added a comment -

Better link to thread describing what I saw with this issue: http://www.nabble.com/JRuby-1.1RC3---Goldspike-1.6-Fedora-6-problem-td16232224.html

Show
Mike Herrick added a comment - Better link to thread describing what I saw with this issue: http://www.nabble.com/JRuby-1.1RC3---Goldspike-1.6-Fedora-6-problem-td16232224.html
Hide
Charles Oliver Nutter added a comment -

Moving to 1.1. This is critical enough to promote it now, so we'll merge the fix to 1.1 tomorrow and push it out with the 1.1 release this weekend. Additional testing after promotion to a temporary 1.1 branch would be extremely welcome.

Show
Charles Oliver Nutter added a comment - Moving to 1.1. This is critical enough to promote it now, so we'll merge the fix to 1.1 tomorrow and push it out with the 1.1 release this weekend. Additional testing after promotion to a temporary 1.1 branch would be extremely welcome.
Hide
Mike Herrick added a comment -

I'll be happy to test ASAP when you are ready. I'll make it my first priority. Like I said, we release our R1 of our app Monday and I'd love to be on 1.1.

Show
Mike Herrick added a comment - I'll be happy to test ASAP when you are ready. I'll make it my first priority. Like I said, we release our R1 of our app Monday and I'd love to be on 1.1.
Hide
Charles Oliver Nutter added a comment -

I think the correct change for this is not to remove the == check entirely, but to change it to break out of the loop of read == 0. In a case where we have nonblocking IO and there's nothing available to read, it's certainly possible that a read might return no bytes. If we remove the check completely, the bufferedRead loop would run forever or until the stream had available data...defeating nonblocking behavior.

I'll tidy up a patch, apply it to 1.1, and post it here for any remaining discussion.

Show
Charles Oliver Nutter added a comment - I think the correct change for this is not to remove the == check entirely, but to change it to break out of the loop of read == 0. In a case where we have nonblocking IO and there's nothing available to read, it's certainly possible that a read might return no bytes. If we remove the check completely, the bufferedRead loop would run forever or until the stream had available data...defeating nonblocking behavior. I'll tidy up a patch, apply it to 1.1, and post it here for any remaining discussion.
Hide
Charles Oliver Nutter added a comment -

Ok, here's the patch:

http://pastie.org/171947

The idea here is that the read loop will keep reading as long as it reads more than zero bytes. If it reads zero bytes the loop terminates and it returns what it got. If the read returns -1 for EOF, it set the EOF flag and returns what it got, raising an error if it's both EOF and nothing was read.

This should fix the original issue, where it would give up too early reading from a stream that had more available, but also allow nonblocking reads to safely return zero bytes for a given loop without running forever.

Tests pass on my end, things are looking ok. Comments before we push this out?

Show
Charles Oliver Nutter added a comment - Ok, here's the patch: http://pastie.org/171947 The idea here is that the read loop will keep reading as long as it reads more than zero bytes. If it reads zero bytes the loop terminates and it returns what it got. If the read returns -1 for EOF, it set the EOF flag and returns what it got, raising an error if it's both EOF and nothing was read. This should fix the original issue, where it would give up too early reading from a stream that had more available, but also allow nonblocking reads to safely return zero bytes for a given loop without running forever. Tests pass on my end, things are looking ok. Comments before we push this out?
Hide
mctozzy added a comment -

Seems reasonable Charles. So the fact that it wasn't previously setting the eof flag upon read==-1, was that a separate bug?

Show
mctozzy added a comment - Seems reasonable Charles. So the fact that it wasn't previously setting the eof flag upon read==-1, was that a separate bug?
Hide
Mike Herrick added a comment -

I'll apply the patch to RC3 and test shortly.

Show
Mike Herrick added a comment - I'll apply the patch to RC3 and test shortly.
Hide
Vladimir Sizikov added a comment -

Actually, Mike, Charlie has just created a jruby-1_1 branch, and all the critical fixes are there.

So it would probably make sense to try that branch.

Show
Vladimir Sizikov added a comment - Actually, Mike, Charlie has just created a jruby-1_1 branch, and all the critical fixes are there. So it would probably make sense to try that branch.
Hide
Vladimir Sizikov added a comment -

Forgot to add, that there were a bit more changes on top of that patch listed above.

Show
Vladimir Sizikov added a comment - Forgot to add, that there were a bit more changes on top of that patch listed above.
Hide
Mike Herrick added a comment -

Glad I mentioned it! Ok, I'll grab the branch and test within the hour. Are the other changes listed somewhere? Thanks. This is likely what we'll be releasing with. Timing splendid

Show
Mike Herrick added a comment - Glad I mentioned it! Ok, I'll grab the branch and test within the hour. Are the other changes listed somewhere? Thanks. This is likely what we'll be releasing with. Timing splendid
Hide
Charles Oliver Nutter added a comment -

Fixed on trunk and on the temporary 1.1 branch. This will be in the 1.1 release.

Show
Charles Oliver Nutter added a comment - Fixed on trunk and on the temporary 1.1 branch. This will be in the 1.1 release.
Hide
Vladimir Sizikov added a comment -

All the changes in 1.1 branch can be seen here:
http://svn.jruby.codehaus.org/changelog/jruby/branches/jruby-1_1

Show
Vladimir Sizikov added a comment - All the changes in 1.1 branch can be seen here: http://svn.jruby.codehaus.org/changelog/jruby/branches/jruby-1_1
Hide
Mike Herrick added a comment -

Thanks. Worked locally, committing the new .jar to our SVN repo now, Hudson CI will take it from there. Will let you know how it goes.

Show
Mike Herrick added a comment - Thanks. Worked locally, committing the new .jar to our SVN repo now, Hudson CI will take it from there. Will let you know how it goes.
Hide
Mike Herrick added a comment -

Works. Thanks so much for getting this in. As we emerge from the panic of our first release, we'll pay you back with more contributions to the JRuby community.

Cheers, Mike

Show
Mike Herrick added a comment - Works. Thanks so much for getting this in. As we emerge from the panic of our first release, we'll pay you back with more contributions to the JRuby community. Cheers, Mike

People

Vote (2)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: