New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail only single request on too large frame #102

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@arhimondr
Copy link
Contributor

arhimondr commented Dec 13, 2018

Resolves #99

@arhimondr arhimondr force-pushed the arhimondr:fail-single-request-on-too-large-frame branch from 4697d85 to 123c62e Dec 13, 2018

@airlift airlift deleted a comment from arhimondr Dec 14, 2018

@arhimondr arhimondr force-pushed the arhimondr:fail-single-request-on-too-large-frame branch from 123c62e to 1d69efa Dec 20, 2018

@arhimondr arhimondr force-pushed the arhimondr:fail-single-request-on-too-large-frame branch from 1d69efa to 242824c Dec 21, 2018

@dain
Copy link
Member

dain left a comment

Some initial comments

Show resolved Hide resolved ...c/test/java/io/airlift/drift/integration/guice/TestGuiceIntegration.java Outdated
Show resolved Hide resolved ...test/java/io/airlift/drift/integration/guice/ThrowingServiceHandler.java Outdated
Show resolved Hide resolved ...c/main/java/io/airlift/drift/transport/netty/codec/FrameInfoDecoder.java Outdated
Show resolved Hide resolved ...rc/main/java/io/airlift/drift/transport/netty/codec/HeaderTransport.java Outdated
Show resolved Hide resolved ...rc/main/java/io/airlift/drift/transport/netty/codec/HeaderTransport.java Outdated
}

byte protocolId = buffer.getByte(buffer.readerIndex());
Protocol protocol = Protocol.getProtocolByHeaderTransportId(protocolId);

This comment has been minimized.

@dain

dain Jan 10, 2019

Member

This can throw.

This comment has been minimized.

@arhimondr

arhimondr Jan 11, 2019

Contributor

It can throw only if client/server tries to use unsupported protocol. In that case there's nothing else we can do, but close the connection anyway.

@arhimondr arhimondr force-pushed the arhimondr:fail-single-request-on-too-large-frame branch from 242824c to c36a79e Jan 11, 2019

@dain
Copy link
Member

dain left a comment

Mostly just comments, suggestions, and questions. You don't need to make all the suggest changes, but I'd like to hear your thoughts either way.

default:
throw new IllegalArgumentException("Unsupported header flags: " + flags);
}
boolean outOfOrderResponse = isOutOrOrderResponseSupported(flags);

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

So here we still need to fail if the flag is unsupported... I was suggesting that in the when skipping frames that are too large we don't need to we can ignore unsupported flags since we are just skipping.

@@ -17,10 +17,10 @@

import io.airlift.drift.protocol.TTransportException;

public class FrameTooLargeException
public class MessageTooLargeException

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

Why rename this?

This comment has been minimized.

@arhimondr

arhimondr Jan 22, 2019

Contributor

Just to avoid name clash. FrameTooLargeException now extends the DecoderException, and indicates max frame size overflow in FrameDecoder. MessageTooLargeException is a TTransportException.

ByteBuf decoded = decode(decoder, buffer);
assertNull(decoded);

buffer.writeByte(0xAB);

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

add some comment about what each of these does. For example, this one writes a partial frame length.

try (TestingPooledByteBufAllocator allocator = new TestingPooledByteBufAllocator()) {
ByteBuf buffer = allocator.buffer(1024);

buffer.writeInt(first.length);

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

maybe add a helper method writeLengthPrefixedFrame(first)

assertEquals(buffer.readerIndex(), 0);
assertEquals(buffer.writerIndex(), Integer.BYTES + 5);

buffer.writeBytes(first, 5, 5);

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

maybe first.length - 5

{
ctx.close();
if (cause instanceof FrameTooLargeException) {

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

Add a comment about what is going on here

}
}

private Object decode(ByteBuf buffer)

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

Change this and its usage to ByteBuf

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

Actually, how about Optional<ByteBuf> and then the use to decode(buffer).ifPresent(output::add)?

This comment has been minimized.

@arhimondr

arhimondr Jan 22, 2019

Contributor

I thought about this as of a hot spot. But i agree that the cost of creating a one Optional object per request is negligible comparing to the cost of decoding and processing.

if (bytesToDiscard == 0) {
RuntimeException exception = new FrameTooLargeException(tooLongFrameInfo, tooLongFrameSizeInBytes, maxFrameSizeInBytes);
tooLongFrameInfo = Optional.empty();
tooLongFrameSizeInBytes = -1;

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

I'd reset this to zero since that is the field default

/**
* Attempts to decode basic frame info without moving the reader index
*/
Optional<FrameInfo> tryDecodeFrameInfo(ByteBuf buffer);

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

I believe this only decodes the Message, and doesn't contain info about the frame size, so maybe we rename all of this Message instead of Frame?

This comment has been minimized.

@arhimondr

arhimondr Jan 22, 2019

Contributor

It also contains the information about the Frame encoding and about the underlying protocol (transport, protocol).


import java.util.Optional;

interface FrameInfoDecoder

This comment has been minimized.

@dain

dain Jan 14, 2019

Member

Can you add a comment (or pick a name) to this and the related class that makes it clear they are only present to deal with large frames. I have a feeling that I'll be confused in the future.... maybe is makes sense to put this stuff in another package also

This comment has been minimized.

@arhimondr

arhimondr Jan 22, 2019

Contributor

Technically it can be used to decode some basic information about any incoming frame. Not sure if it makes sense to change the make to explicitly emphasize how it is being used. What about adding a comment?

@arhimondr arhimondr force-pushed the arhimondr:fail-single-request-on-too-large-frame branch from c36a79e to 857671c Jan 23, 2019

@arhimondr

This comment has been minimized.

Copy link
Contributor

arhimondr commented Jan 23, 2019

@dain updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment