reactor / reactor-netty

TCP/HTTP/UDP/QUIC client/server with Reactor over Netty
https://projectreactor.io
Apache License 2.0
2.57k stars 641 forks source link

Unable to handle http/2 (deprecated) PRIORITY frames #3330

Closed bmaassenee closed 2 months ago

bmaassenee commented 3 months ago

I came across a http/2 server that send a (deprecated) PRIORITY frame https://datatracker.ietf.org/doc/html/rfc7540#section-6.3. This caused the client to fail reading the response with an IllegalArgumentException in ByteBufFlux.bytebufExtractor

Caused by: java.lang.IllegalArgumentException: Object DefaultHttp2PriorityFrame(stream=3, streamDependency=0, weight=16, exclusive=false) of type class io.netty.handler.codec.http2.DefaultHttp2PriorityFrame cannot be converted to ByteBuf
    at reactor.netty.ByteBufFlux.lambda$static$10(ByteBufFlux.java:363) ~[reactor-netty-core-1.0.46.jar:1.0.46]

Expected Behavior

I belief that this "meta" frame should not be sent as part of the response body Flux, but instead should be handled/ignored by the client

Actual Behavior

Steps to Reproduce

Unfortunately the server i encountered this, is not a public server. So no easy reproduction is possible i'm afraid.

Possible Solution

HttpClientOperations.onInboundNext could drop the msg object if its an instance of Http2PriorityFrame instead of passing it along to super.onInboundNext(ctx, msg);

Your Environment

violetagg commented 3 months ago

@bmaassenee That is very very strange. Reactor Netty always works with HttpContent abstraction. We always convert the frames, see https://github.com/reactor/reactor-netty/blob/703bfc6c054f907d8b81b781b5cb08ecb685bfbb/reactor-netty-http/src/main/java/reactor/netty/http/client/HttpClientConfig.java#L592

Have you changed the Reactor Netty pipeline in some way?

bmaassenee commented 3 months ago

no i haven't. This is basically the full setup(spring webclient using netty http)

ConnectionProvider provider = ConnectionProvider.builder( "spring-webclient" )
        // this defines the maximum number of iterable requests from flux allowed, we configure no limit, but instead a short timeout
        .pendingAcquireMaxCount( -1 )
        // allow up to 500 parallel connections
        .maxConnections( 500 )
        // Give up if we can't get a connection within 10 sec
        .pendingAcquireTimeout( Duration.ofSeconds( 10 ) )
        // close connections if not used for 30 sec
        .maxIdleTime( Duration.ofSeconds( 30 ) )
        // check connections of the pool every min to see if we can close them
        .evictInBackground( Duration.ofSeconds( 60 ) )
        // use the last returned connection first, this way we keep our pool lean
        .lifo()
        // enables metrics https://projectreactor.io/docs/netty/release/reference/index.html#_metrics_5
        .metrics( true )
        .build();

HttpClient httpClient = HttpClient.create( provider );

if( !validateSSL )
{
    SslContext sslContext = SslContextBuilder
            .forClient()
            .trustManager( InsecureTrustManagerFactory.INSTANCE )
            .build();
    httpClient = httpClient
            .secure( sslContextSpec -> sslContextSpec.sslContext( sslContext ) );
}

//set default read timeout
httpClient = httpClient
        //show raw requests in logs if you add <ASyncLogger name="reactor.netty.http.client" level="trace"/> to the log config
        .wiretap( "reactor.netty.http.client.HttpClient", LogLevel.TRACE, AdvancedByteBufFormat.TEXTUAL )
        .resolver( DefaultAddressResolverGroup.INSTANCE )//TODO this is added to debug INFRA-1358 by using a different dns resolver
        .option( ChannelOption.CONNECT_TIMEOUT_MILLIS, StandardTimeouts.V3_CONNECTION_TIMEOUT_MS )
        .headers( h -> h.set( HttpHeaders.USER_AGENT, createUserAgent() ) )
        .compress( true )
        .responseTimeout( Duration.ofMillis( connectionTimeout ) );

httpClient = httpClient
        .protocol( HttpProtocol.H2C )
        .noSSL();

httpClient
        .warmup()
        .subscribe();

ExchangeStrategies strategies = ExchangeStrategies.builder()
        .codecs( clientDefaultCodecsConfigurer -> {
            clientDefaultCodecsConfigurer.defaultCodecs().maxInMemorySize( VmsWebClientSettings.WEB_CLIENT_MAX_IN_MEMORY_SIZE );
        } ).build();

return WebClient.builder()
        .defaultHeader( "Accept", "*/*" )
        .clientConnector( new ReactorClientHttpConnector( httpClient ) )
        .exchangeStrategies( strategies )
        .build();

And making the call:

_client.get().uri( "/internal/livestream/{deviceId}", uri -> uri
        .scheme( "http" )
        .host( host )
        .port( 8080 )
        .queryParam( "type", "multipart" )
        .build( deviceId )
)
.retrieve()
bmaassenee commented 3 months ago

But with the refence you gave, it gave me the option to debug a little further and found that the acceptInboundMessage of the codec doesn't accept the PriorityFrame: image

violetagg commented 3 months ago

@violetagg Thanks a lot! Let me take over from now on

violetagg commented 2 months ago

@bmaassenee Please follow this PR in Netty https://github.com/netty/netty/pull/14168