Skip to content
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

Possible memory leak in HttpServerHandler and FluxReceive #406

Closed
kadledav opened this issue Aug 2, 2018 · 9 comments
Closed

Possible memory leak in HttpServerHandler and FluxReceive #406

kadledav opened this issue Aug 2, 2018 · 9 comments
Labels
type/bug A general bug
Milestone

Comments

@kadledav
Copy link

kadledav commented Aug 2, 2018

I was searching for memory leak by Netty in our application (org.springframework.boot:2.0.3.RELEASE , reactor-netty:0.7.8.RELEASE)

First:
I have discovered a suspicious code in the reactor.ipc.netty.http.server.HttpServerHandler#channelRead
in the case that a following expression is true:
msg instanceof HttpRequest AND decoderResult.isFailure()
there is no ReferenceCountUtil.release(msg) and msg is also not send for other processing. So I think there is a possible memory leak.

Second:
Unfortunately that wasn't my case. I have found out that problem is in the reactor.ipc.netty.channel.FluxReceive . It seems that sometimes there were some messages in the
receiverQueue and release() on those messages was never called. So I have made an ugly patch (added finally block) in reactor.ipc.netty.channel.ChannelOperations#applyHandler

	protected final void applyHandler() {
		//		channel.pipeline()
		//		       .fireUserEventTriggered(NettyPipeline.handlerStartedEvent());
		if (log.isDebugEnabled()) {
			log.debug("[{}] {} handler is being applied: {}", formatName(), channel
					(), handler);
		}
		try {
			Mono.fromDirect(handler.apply((INBOUND) this, (OUTBOUND) this))
					.subscribe(this);
		}
		catch (Throwable t) {
			log.error("", t);
			channel.close();
		// FINALLY SECTION BELOW WAS ADDED
		} finally {
			if (inbound != null) {
				inbound.cleanQueue(inbound.receiverQueue);
			}
		}
	}

The code above helped me and Netty doesn't report memory leak anymore.
Unfortunately I am not able to write simple test to reproduce it.

Reactor Netty version

0.7.8.RELEASE

JVM version

jdk-10.0.1

OS version

Windows 10

@violetagg violetagg added the type/bug A general bug label Aug 6, 2018
@violetagg violetagg added this to the 0.7.9.RELEASE milestone Aug 6, 2018
@violetagg
Copy link
Member

violetagg commented Aug 6, 2018

@kadledav Can you provide some description of the scenario that you think causes the leak? Do you consume the whole response body, or you receive some error? Do you abandon the response body and consume only the status code/headers?

For the first issue I agree that it is a bug and will be fixed.

violetagg added a commit that referenced this issue Aug 6, 2018
@violetagg violetagg added the for/user-attention This issue needs user attention (feedback, rework, etc...) label Aug 7, 2018
@violetagg
Copy link
Member

@kadledav Can you provide more information about the leak scenario?

@kadledav
Copy link
Author

The leak scenario is quite simple. I have exposed our application to a HTTP stress test and waited until ResourceLeakDetector reported a memory leak. But I am not able to reproduce it with a simple spring-boot application. Our application has more complicated Netty pipeline because we are also using a legacy protocol on the same port as HTTP for legacy clients.

I think I have found out where the problem could be. This memory leak issue has started since we have updated spring-boot from 2.0.1 to 2.0.2 (regardless I force the reactor-netty version). I think it could be related to some incompatibilities in our adjusted libraries. The problem still persist with spring-boot 2.0.4.

Even though the problem is not directly caused by reactor-netty project there was an unreleased direct memory object which caused memory leak which. I think the code working with direct memory should be robust and always clean it's memory.

I have encountered a similar problem before, when we have a wrong implementation of
org.springframework.web.server.WebExceptionHandler . We have tried to render our custom page on every exception. But in a case when response was already commited (java.io.IOException: An established connection was aborted by the software in your host machine ) Netty reported a memory leak in a few seconds.

Again I am not able to reproduce it with a simple spring-boot application. So I will continue searching what is causing the the memory leak in our application but not in a simple example.

@violetagg
Copy link
Member

@kadledav Can you try reproducing the issue with -Dio.netty.leakDetectionLevel=paranoid and also increase Reactor Netty log level to DEBUG with adding logging.level.reactor.ipc.netty=DEBUG to the application.properties. You should receive stack traces like these http://netty.io/wiki/reference-counted-objects.html#troubleshooting-buffer-leaks

@kadledav
Copy link
Author

I have reproduced a memory leak and attached a log file. In the log file are all requests from server start until a memory leak occurred . memoryLeak.log

Scenario:
Login page with two other resources (css and fonts). I was quickly pressing F5 in the browser until memory leak was reported.
In the other test I was pressing F5 slowly (always waited until page is loaded) and memory leak did not occur. So I think the problem is related to kicked connections.

@violetagg
Copy link
Member

@kadledav What did you say the Reactor Netty version is?
I see the following class reactor.ipc.netty.http.server.HttpProtocolPipelineBuilder in the log file which does not exists in the current repository.

@kadledav
Copy link
Author

Hello,

I have finally find out what is causing the memory leak. We are using our Netty ServerBootstrap and based on the communication we are modifying the pipeline to support desired protocol. Unfortunately we cannot start always with HTTP and do upgrade protocol with 101 status code due to our legacy clients, which doesn't support HTTP communication.

Our problem: We need to modify our pipeline to HTTP if it's HTTP communication. So we created a solution which has some issues, because we are not able to easily build a HTTP pipeline with reactor-netty API. Basically we have looked into a reactor-netty code and used ContextHandler.newServerContext( ...); . This is the problem, we have a different handlers then reactor-netty code has nowadays.

There is a class example providing ChannelInitializer<Channel> for our pipeline.

package reactor.ipc.netty.http.server;

import java.time.Duration;

import org.springframework.http.server.reactive.ReactorHttpHandlerAdapter;
import org.springframework.web.server.adapter.HttpWebHandlerAdapter;

import io.netty.channel.Channel;
import io.netty.channel.ChannelInitializer;
import io.netty.channel.ChannelPipeline;
import io.netty.handler.codec.http.HttpObjectAggregator;
import io.netty.handler.codec.http.HttpRequestDecoder;
import io.netty.handler.codec.http.HttpResponseEncoder;
import io.netty.handler.codec.http.HttpServerCodec;
import io.netty.handler.logging.LoggingHandler;
import reactor.core.publisher.Mono;
import reactor.ipc.netty.NettyContext;
import reactor.ipc.netty.NettyPipeline;
import reactor.ipc.netty.channel.ContextHandler;

public class HttpProtocolPipelineBuilder {

	private final LoggingHandler loggingHandler = new LoggingHandler(HttpProtocolPipelineBuilder.class);
	private ChannelInitializer<Channel> channelInitializer;
	private boolean withMemoryLeak = true;

	public HttpProtocolPipelineBuilder(HttpWebHandlerAdapter httpWebHandlerAdapter) {
		Mono.<NettyContext>create(sink -> {
			HttpServerOptions options = HttpServerOptions.builder().build();
			channelInitializer = ContextHandler.newServerContext(sink, options, loggingHandler,
					(ch, c, msg) -> HttpServerOperations.bindHttp(ch, new ReactorHttpHandlerAdapter(httpWebHandlerAdapter), c, null, msg))
					.onPipeline(this::buildPipeline)
					.autoCreateOperations(false);
			sink.success();
		}).block(Duration.ofSeconds(10));
	}

	public ChannelInitializer<Channel> getChannelInitializer() {
		return channelInitializer;
	}

	private void buildPipeline(ChannelPipeline p, ContextHandler<Channel> c) {
		if (withMemoryLeak) {
			p.addLast("reactor.left.httpDecoder", new HttpRequestDecoder());
			p.addLast(NettyPipeline.HttpAggregator, new HttpObjectAggregator(Integer.MAX_VALUE));
			p.addLast("reactor.left.httpEncoder", new HttpResponseEncoder());
			p.addLast(NettyPipeline.HttpServerHandler, new HttpServerHandler(c));
		} else {
			p.addLast(NettyPipeline.HttpCodec, new HttpServerCodec());
			p.addLast(NettyPipeline.HttpServerHandler, new HttpServerHandler(c));
		}
	}
}

I know that our approach is ugly at least because those reasons:

  • we need to use a reactor.ipc.netty.http.server package name so we can access HttpServerOperations
  • we are creating a pipeline in our code, so it is not updated when we update reactor-netty library

Is there a more elegant way to retrieve HTTP pipeline without building and binding reactor-netty HttpServer ? I am very sorry to bother you with the 'memory leak problem' since it was our issue. Thank you for your time and patience.

@violetagg
Copy link
Member

Hm you are able to manipulate the pipeline with the following two possibilities depending on when you want to attach the handlers:
afterChannelInit and afterNettyContextInit
Here https://github.com/reactor/reactor-netty/blob/0.7.x/src/test/java/reactor/ipc/netty/options/NettyOptionsTest.java#L141 you can find and example how this can be done.
Do you think this will help?

@kadledav
Copy link
Author

Yes, this helps a lot. Thank you.

@violetagg violetagg removed the for/user-attention This issue needs user attention (feedback, rework, etc...) label Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants