Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Date: Wed, 22 Jul 2015 12:26:04 +0000 (UTC)
From: "Benedict (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12846857.1437561528000.255766.1437567964952@Atlassian.JIRA>
In-Reply-To: <JIRA.12846857.1437561528000@Atlassian.JIRA>
References: <JIRA.12846857.1437561528000@Atlassian.JIRA>
 <JIRA.12846857.1437561528069@arcas>
Subject: [jira] [Comment Edited] (CASSANDRA-9863) NIODataInputStream has
 problems on trunk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/CASSANDRA-9863?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14636788#comment-14636788 ] 

Benedict edited comment on CASSANDRA-9863 at 7/22/15 12:25 PM:
---------------------------------------------------------------

It's a "costly" no-op that under correct usage we never incur, but without any code complexity results in correct behaviour under incorrect usage, i.e. throwing an exception (except in this case, which is absolutely nothing to do with {{readNext}}, so modifying it off the back of it does not seem a justifiable course of action). The alternative is making every _correct_ usage that does perform this more costly and - more importantly - more confusing, as the code you're suggesting entails more special casing. I don't want to try and ensure we miss a null or some other behaviour somewhere where we misinterpret correct behaviour on empty, when we can just always get that correct behaviour for free by providing an empty RBC.

As far as the ByteBuffer interface itself being ugly to work with, re: flips, etc. I agree, but there's really not much to be done about that.

edit: Hmm. Rethinking that, you're right that vints change this a little. Not sure they change it often enough to be worth modifying the code and incurring that cost/risk of having null / broken behaviour on empty, however.

edit2: Except no, it doesn't. If we go with the slightly modified approach to CASSANDRA-9708 I was planning to propose, which is always ensuring there are at least 8 bytes spare at the end of any ByteBuffer. So we never have to extend and call the empty RBC.


was (Author: benedict):
-It's a "costly" no-op that under correct usage we never incur, but without any code complexity results in correct behaviour under incorrect usage, i.e. throwing an exception (except in this case, which is absolutely nothing to do with {{readNext}}, so modifying it off the back of it does not seem a justifiable course of action). The alternative is making every _correct_ usage that does perform this more costly and - more importantly - more confusing, as the code you're suggesting entails more special casing. I don't want to try and ensure we miss a null or some other behaviour somewhere where we misinterpret correct behaviour on empty, when we can just always get that correct behaviour for free by providing an empty RBC.-

-As far as the ByteBuffer interface itself being ugly to work with, re: flips, etc. I agree, but there's really not much to be done about that.-

Hmm. Rethinking that, you're right that vints change this a little. Not sure they change it often enough to be worth modifying the code and incurring that cost/risk of having null / broken behaviour on empty, however.


> NIODataInputStream has problems on trunk
> ----------------------------------------
>
>                 Key: CASSANDRA-9863
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-9863
>             Project: Cassandra
>          Issue Type: Bug
>            Reporter: Sylvain Lebresne
>            Assignee: Ariel Weisberg
>            Priority: Blocker
>             Fix For: 3.0 beta 1
>
>
> As the title says, there is cases where method calls to NIODataInputStream, at least {{readVInt}} calls can loop forever. This is possibly only a problem for vints where the code tries to read 8 bytes minimum but there is less than that available, and in that sense is related to [~benedict]'s observation in CASSANDRA-9708, but it is more serious than said observation because:
> # this can happen even if the buffer passed to NIODataInputStream ctor has more than 8 bytes available, and hence I'm relatively confident [~benedict]'s fix in CASSANDRA-9708 is not enough.
> # this doesn't necessarily fail cleanly by raising assertions, this can loop forever (which is much harder to debug).
> Due of that, and because that is at least one of the cause of CASSANDRA-9764, I think the problem warrants a specific ticket (from CASSANDRA-9708 that is).
> Now, the exact reason of this is looping is if {{readVInt}} is called but the buffer has less than 8 byte remaining (again, the buffer had more initially). In that case, {{readMinimum(8, 1)}} is called and it calls {{readNext()}} in a loop. Within {{readNext()}}, the buffer (which has {{buf.position() == 0 && buf.hasRemaining()}}) is actually unchanged (through a very weird dance of setting the position to the limit, then the limit to the capacity, and then flipping the buffer which resets everything to what it was), and because {{rbc}} is the {{emptyReadableByteChannel}}, {{rbc.read(buf)}} does nothing and always return {{-1}}. Back in {{readMinimum}}, {{read == -1}} but {{remaining >= require}} (and {{remaining}} never changes), and hence the forever looping.
> Now, not sure what the best fix is because I'm not fully familiar with that code, but that does leads me to a 2nd point: {{NIODataInputSttream}} can IMHO use a bit of additional/better comments. I won't pretend having tried very hard to understand the whole class, so there is probably some lack of effort, but at least a few things felt like they should clarified:
> * Provided I understand {{readNext()}} correctly, it only make sense when we do have a {{ReadableByteChannel}} (and the fact that it's not the case sounds like the bug). If that's the case, this should be explicitly documented and probably asserted. As as an aside, I wonder if using {{rbc == null}} when we don't have wouldn't be better: if we don't have one, we shouldn't try to use it, and having a {{null}} would make things fail loudly if we do.
> * I'm not exactly sure what {{readMinimum}} arguments do. I'd have expected at least one to be called "minimum", and an explanation of the meaning of the other one.
> * {{prepareReadPaddedPrimitive}} says that it "Add padding if requested" but there is seemingly no argument that trigger the "if requested part". Also unclear what that padding is about in the first place.
> As a final point, it looks like the case where {{NIODataInputStream}} is constructed with a {{ByteBuffer}} (rather than a {{ReadableByteChannel}}) seems to be completely untested by the unit tests.


--
This message was sent by Atlassian JIRA
(v6.3.4#6332)