cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ariel Weisberg (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-9863) NIODataInputStream has problems on trunk
Date Thu, 23 Jul 2015 15:07:05 GMT


Ariel Weisberg commented on CASSANDRA-9863:

I updated the branch. There is a derived class for wrapping fixed immutable buffers without
a channel and it ensures that no shuffling takes place by overriding readNext(). It also throws
AssertionError if it gets far enough to try reading from the empty channel. You should never
see the AssertionError since read next always returns -1 and doesn't touch the channel.

For varints if there are < 9 bytes remaining in the buffer it uses the slow path in VIntCoding
that reads byte at a time.

> NIODataInputStream has problems on trunk
> ----------------------------------------
>                 Key: CASSANDRA-9863
>                 URL:
>             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
> # 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}}, {{}} 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

View raw message