Issue Details (XML | Word | Printable)

Key: JRUBY-2314
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Critical Critical
Assignee: Charles Oliver Nutter
Reporter: mctozzy
Votes: 2
Watchers: 5
Operations

If you were logged in you would be able to see more operations.
JRuby

Buffer management defect in RubyIO library: Class ChannelStream

Created: 24/Mar/08 05:23 PM   Updated: 07/Apr/08 04:00 PM
Component/s: Core Classes/Modules
Affects Version/s: JRuby 1.1RC3
Fix Version/s: JRuby 1.1

Time Tracking:
Not Specified

Environment: All

Patch Submitted: Yes


 Description  « Hide
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?



 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Mike Herrick added a comment - 24/Mar/08 05:27 PM
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)


Charles Oliver Nutter added a comment - 24/Mar/08 06:54 PM
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).

Mike Herrick added a comment - 24/Mar/08 07:06 PM
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.

Mike Herrick added a comment - 24/Mar/08 10:01 PM
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

Charles Oliver Nutter added a comment - 24/Mar/08 10:19 PM
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.

Mike Herrick added a comment - 24/Mar/08 10:23 PM
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.


Charles Oliver Nutter added a comment - 28/Mar/08 04:52 AM
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.


Charles Oliver Nutter added a comment - 28/Mar/08 05:16 AM
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?


mctozzy added a comment - 28/Mar/08 05:26 AM
Seems reasonable Charles. So the fact that it wasn't previously setting the eof flag upon read==-1, was that a separate bug?

Mike Herrick added a comment - 28/Mar/08 08:34 AM
I'll apply the patch to RC3 and test shortly.

Vladimir Sizikov added a comment - 28/Mar/08 08:41 AM
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.


Vladimir Sizikov added a comment - 28/Mar/08 08:42 AM
Forgot to add, that there were a bit more changes on top of that patch listed above.

Mike Herrick added a comment - 28/Mar/08 08:44 AM
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

Charles Oliver Nutter added a comment - 28/Mar/08 08:47 AM
Fixed on trunk and on the temporary 1.1 branch. This will be in the 1.1 release.

Vladimir Sizikov added a comment - 28/Mar/08 08:51 AM
All the changes in 1.1 branch can be seen here:
http://svn.jruby.codehaus.org/changelog/jruby/branches/jruby-1_1

Mike Herrick added a comment - 28/Mar/08 08:58 AM
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.

Mike Herrick added a comment - 28/Mar/08 09:11 AM
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