Potential Performance Issue, exception in normal path

[jasperpotts]

Description

I was reading generated protobuf parser code and saw the comment // There's no more fields. Stop the parsing loop. and it made we wonder if this is happening in normal parsing. If it is then we should add a if check on remaining() and avoid the exception for normal operations.

            final int tag;
            try {
                // Read the "tag" byte which gives us the field number for the next field to read
                // and the wire type (way it is encoded on the wire).
                tag = input.readVarInt(false);
            } catch (EOFException e) {
                // There's no more fields. Stop the parsing loop.
                break;
            }

Steps to reproduce

Run parsing test with debugger on the break; line and find if it ever gets reached.

Additional context

No response

Hedera network

No response

Version

0.12.10

Operating system

None

[anthony-swirldslabs]

@jasperpotts : There's a hasRemaining() check. Expanding the code snippet a few lines above, here's how the loop header actually looks (taken from AccountIDProtoCodec):

                // Continue to parse bytes out of the input stream until we get to the end.
                while (input.hasRemaining()) {
                    // Note: ReadableStreamingData.hasRemaining() won't flip to false
                    // until the end of stream is actually hit with a read operation.
                    // So we catch this exception here and **only** here, because an EOFException
                    // anywhere else suggests that we're processing malformed data and so
                    // we must re-throw the exception then.
                    final int tag;
                    try {
                        // Read the "tag" byte which gives us the field number for the next field to read
                        // and the wire type (way it is encoded on the wire).
                        tag = input.readVarInt(false);
                    } catch (EOFException e) {
                        // There's no more fields. Stop the parsing loop.
                        break;
                    }
        
...

Right before reading the tag, the loop condition verifies that the input has remaining bytes. Please note that it's impossible to check for the exact number of remaining bytes because the tag is a varint. So we're performing the best check that we possibly can, and if the input is a correct protobuf, then this loop will never run into the EOFException.

As a comment in this code snippet suggests, our ReadableStreamingData.hasRemaining() may not be able to detect an EOF situation until a real read operation hits the actual end of stream. Here's how it looks today:

    /** {@inheritDoc} */
    @Override
    public long remaining() {
        return eof ? 0 : limit - position;
    }

    /** {@inheritDoc} */
    @Override
    public boolean hasRemaining() {
        return !eof && position < limit;
    }

I've also added the remaining() implementation to demonstrate that it wouldn't add value over the hasRemaining() even if we knew the exact number of expected bytes (which we don't, as I said above) because its implementation is basically equivalent to the hasRemaining(). I believe that the position < limit condition helps avoid unwanted EOFException in most cases. But when it doesn't, then the EOFException is our only way out here. Here's where it may be produced (see the very middle of the snippet):

    @Override
    public long readVarLong(final boolean zigZag) {
        if (!hasRemaining()) {
            throw new BufferUnderflowException();
        }

        long value = 0;

        try {
            int i = 0;
            for (; i < 10; i++) {
                final int b = in.read();
                if (b < 0) {
                    eof = true;
                    throw new EOFException();
                }
                value |= (long) (b & 0x7F) << (i * 7);
                if ((b & 0x80) == 0) {
                    position += i + 1;
                    return zigZag ? (value >>> 1) ^ -(value & 1) : value;
                }
            }
            assert i == 10;
            throw new DataEncodingException("Malformed var int");
        } catch (final IOException e) {
            throw new UncheckedIOException(e);
        }
    }

In other words, our in.read() call to the encapsulated InputStream inside the ReadableStreamingData is the only way to detect the actual EOF state.

I don't see any other API in the InputStream interface that would allow us to detect a real EOF. So apart from the already existing position < limit thing mentioned above, I don't believe there's anything better that we can do here.

Please note that I've also checked the readVarInt() implementations in the BufferedData classes, and they wouldn't throw the EOF exception. They throw the DataEncodingException or BufferUnderflowException instead to indicate malformed data.

Since I don't see any better way to handle this situation in our current ReadableStreamingData implementation, I'll be closing this issue. Please feel free to reopen and provide details if you have better ideas.