[
https://issues.apache.org/jira/browse/CASSANDRA-9863?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14637783#comment-14637783
]
Ariel Weisberg commented on CASSANDRA-9863:
-------------------------------------------
Hmmm yeah... the shuffling is absolutely evil. It would do it for EOF as well.
Seems like we need to differentiate between fixed and non-fixed buffers for
real.
I think a derived class that copies to a thread-local to provide padding and
throws EOF immediately without shuffling would be good?
> 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)
